From 529d886c38e7a0e94d90145a2deb6bc7c35e06e8 Mon Sep 17 00:00:00 2001 From: Aaron Alaniz Date: Tue, 21 Jan 2020 03:09:47 +0000 Subject: [PATCH] Allow DTMF delay configurability This commit enables developers to configure the "," delay value from the WebRTC spec value of 2 seconds. This flexibility allows developers to comply with existing WebRTC clients. Bug: webrtc:11273 Change-Id: Ia94b99e041df882e2396d0926a8f4188afe55885 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/165700 Commit-Queue: Steve Anton Reviewed-by: Steve Anton Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#30354} --- api/dtmf_sender_interface.h | 27 ++++++++++++++++++++++- pc/dtmf_sender.cc | 26 ++++++++++++++-------- pc/dtmf_sender.h | 8 +++++-- pc/dtmf_sender_unittest.cc | 44 ++++++++++++++++++++++++++++--------- 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/api/dtmf_sender_interface.h b/api/dtmf_sender_interface.h index 9cdfba189c..7c0e2ce7c3 100644 --- a/api/dtmf_sender_interface.h +++ b/api/dtmf_sender_interface.h @@ -44,6 +44,9 @@ class DtmfSenderObserverInterface { // See: https://www.w3.org/TR/webrtc/#peer-to-peer-dtmf class DtmfSenderInterface : public rtc::RefCountInterface { public: + // Provides the spec compliant default 2 second delay for the ',' character. + static const int kDtmfDefaultCommaDelayMs = 2000; + // Used to receive events from the DTMF sender. Only one observer can be // registered at a time. UnregisterObserver should be called before the // observer object is destroyed. @@ -71,12 +74,29 @@ class DtmfSenderInterface : public rtc::RefCountInterface { // |inter_tone_gap| must be at least 50 ms but should be as short as // possible. // + // The |comma_delay| parameter indicates the delay after the ',' + // character. InsertDtmf specifies |comma_delay| as an argument + // with a default value of 2 seconds as per the WebRTC spec. This parameter + // allows users to comply with legacy WebRTC clients. The |comma_delay| + // must be at least 50 ms. + // // If InsertDtmf is called on the same object while an existing task for this // object to generate DTMF is still running, the previous task is canceled. // Returns true on success and false on failure. virtual bool InsertDtmf(const std::string& tones, int duration, - int inter_tone_gap) = 0; + int inter_tone_gap) { + return InsertDtmf(tones, duration, inter_tone_gap, + kDtmfDefaultCommaDelayMs); + } + virtual bool InsertDtmf(const std::string& tones, + int duration, + int inter_tone_gap, + int comma_delay) { + // TODO(bugs.webrtc.org/165700): Remove once downstream implementations + // override this signature rather than the 3-parameter one. + return InsertDtmf(tones, duration, inter_tone_gap); + } // Returns the tones remaining to be played out. virtual std::string tones() const = 0; @@ -91,6 +111,11 @@ class DtmfSenderInterface : public rtc::RefCountInterface { // default value of 50 ms if InsertDtmf() was never called. virtual int inter_tone_gap() const = 0; + // Returns the current value of the "," character delay in ms. + // This value will be the value last set via the InsertDtmf() method, or the + // default value of 2000 ms if InsertDtmf() was never called. + virtual int comma_delay() const { return kDtmfDefaultCommaDelayMs; } + protected: ~DtmfSenderInterface() override = default; }; diff --git a/pc/dtmf_sender.cc b/pc/dtmf_sender.cc index af5b80977e..10378028c8 100644 --- a/pc/dtmf_sender.cc +++ b/pc/dtmf_sender.cc @@ -33,8 +33,7 @@ namespace webrtc { // +-------+--------+------+---------+ // The "," is a special event defined by the WebRTC spec. It means to delay for // 2 seconds before processing the next tone. We use -1 as its code. -static const int kDtmfCodeTwoSecondDelay = -1; -static const int kDtmfTwoSecondInMs = 2000; +static const int kDtmfCommaDelay = -1; static const char kDtmfValidTones[] = ",0123456789*#ABCDabcd"; static const char kDtmfTonesTable[] = ",0123456789*#ABCD"; // The duration cannot be more than 6000ms or less than 40ms. The gap between @@ -76,7 +75,8 @@ DtmfSender::DtmfSender(rtc::Thread* signaling_thread, signaling_thread_(signaling_thread), provider_(provider), duration_(kDtmfDefaultDurationMs), - inter_tone_gap_(kDtmfDefaultGapMs) { + inter_tone_gap_(kDtmfDefaultGapMs), + comma_delay_(kDtmfDefaultCommaDelayMs) { RTC_DCHECK(signaling_thread_); if (provider_) { RTC_DCHECK(provider_->GetOnDestroyedSignal()); @@ -107,11 +107,12 @@ bool DtmfSender::CanInsertDtmf() { bool DtmfSender::InsertDtmf(const std::string& tones, int duration, - int inter_tone_gap) { + int inter_tone_gap, + int comma_delay) { RTC_DCHECK(signaling_thread_->IsCurrent()); if (duration > kDtmfMaxDurationMs || duration < kDtmfMinDurationMs || - inter_tone_gap < kDtmfMinGapMs) { + inter_tone_gap < kDtmfMinGapMs || comma_delay < kDtmfMinGapMs) { RTC_LOG(LS_ERROR) << "InsertDtmf is called with invalid duration or tones gap. " "The duration cannot be more than " @@ -130,6 +131,7 @@ bool DtmfSender::InsertDtmf(const std::string& tones, tones_ = tones; duration_ = duration; inter_tone_gap_ = inter_tone_gap; + comma_delay_ = comma_delay; // Clear the previous queue. dtmf_driver_.Clear(); // Kick off a new DTMF task queue. @@ -149,6 +151,10 @@ int DtmfSender::inter_tone_gap() const { return inter_tone_gap_; } +int DtmfSender::comma_delay() const { + return comma_delay_; +} + void DtmfSender::QueueInsertDtmf(const rtc::Location& posted_from, uint32_t delay_ms) { dtmf_driver_.AsyncInvokeDelayed( @@ -180,10 +186,12 @@ void DtmfSender::DoInsertDtmf() { } int tone_gap = inter_tone_gap_; - if (code == kDtmfCodeTwoSecondDelay) { - // Special case defined by WebRTC - The character',' indicates a delay of 2 - // seconds before processing the next character in the tones parameter. - tone_gap = kDtmfTwoSecondInMs; + if (code == kDtmfCommaDelay) { + // Special case defined by WebRTC - By default, the character ',' indicates + // a delay of 2 seconds before processing the next character in the tones + // parameter. The comma delay can be set to a non default value via + // InsertDtmf to comply with legacy WebRTC clients. + tone_gap = comma_delay_; } else { if (!provider_) { RTC_LOG(LS_ERROR) << "The DtmfProvider has been destroyed."; diff --git a/pc/dtmf_sender.h b/pc/dtmf_sender.h index 692c74bcef..e332a7ef58 100644 --- a/pc/dtmf_sender.h +++ b/pc/dtmf_sender.h @@ -56,10 +56,12 @@ class DtmfSender : public DtmfSenderInterface, public sigslot::has_slots<> { bool CanInsertDtmf() override; bool InsertDtmf(const std::string& tones, int duration, - int inter_tone_gap) override; + int inter_tone_gap, + int comma_delay = kDtmfDefaultCommaDelayMs) override; std::string tones() const override; int duration() const override; int inter_tone_gap() const override; + int comma_delay() const override; protected: DtmfSender(rtc::Thread* signaling_thread, DtmfProviderInterface* provider); @@ -83,6 +85,7 @@ class DtmfSender : public DtmfSenderInterface, public sigslot::has_slots<> { std::string tones_; int duration_; int inter_tone_gap_; + int comma_delay_; // Invoker for running delayed tasks which feed the DTMF provider one tone at // a time. rtc::AsyncInvoker dtmf_driver_; @@ -96,10 +99,11 @@ PROXY_SIGNALING_THREAD_DESTRUCTOR() PROXY_METHOD1(void, RegisterObserver, DtmfSenderObserverInterface*) PROXY_METHOD0(void, UnregisterObserver) PROXY_METHOD0(bool, CanInsertDtmf) -PROXY_METHOD3(bool, InsertDtmf, const std::string&, int, int) +PROXY_METHOD4(bool, InsertDtmf, const std::string&, int, int, int) PROXY_CONSTMETHOD0(std::string, tones) PROXY_CONSTMETHOD0(int, duration) PROXY_CONSTMETHOD0(int, inter_tone_gap) +PROXY_CONSTMETHOD0(int, comma_delay) END_PROXY_MAP() // Get DTMF code from the DTMF event character. diff --git a/pc/dtmf_sender_unittest.cc b/pc/dtmf_sender_unittest.cc index 3f59af0e23..f7f229a887 100644 --- a/pc/dtmf_sender_unittest.cc +++ b/pc/dtmf_sender_unittest.cc @@ -133,10 +133,12 @@ class DtmfSenderTest : public ::testing::Test { // Constructs a list of DtmfInfo from |tones|, |duration| and // |inter_tone_gap|. - void GetDtmfInfoFromString(const std::string& tones, - int duration, - int inter_tone_gap, - std::vector* dtmfs) { + void GetDtmfInfoFromString( + const std::string& tones, + int duration, + int inter_tone_gap, + std::vector* dtmfs, + int comma_delay = webrtc::DtmfSender::kDtmfDefaultCommaDelayMs) { // Init extra_delay as -inter_tone_gap - duration to ensure the first // DtmfInfo's gap field will be 0. int extra_delay = -1 * (inter_tone_gap + duration); @@ -147,7 +149,7 @@ class DtmfSenderTest : public ::testing::Test { int code = 0; webrtc::GetDtmfCode(tone, &code); if (tone == ',') { - extra_delay = 2000; // 2 seconds + extra_delay = comma_delay; } else { dtmfs->push_back(FakeDtmfProvider::DtmfInfo( code, duration, duration + inter_tone_gap + extra_delay)); @@ -165,11 +167,14 @@ class DtmfSenderTest : public ::testing::Test { } // Verify the provider got all the expected calls. - void VerifyOnProvider(const std::string& tones, - int duration, - int inter_tone_gap) { + void VerifyOnProvider( + const std::string& tones, + int duration, + int inter_tone_gap, + int comma_delay = webrtc::DtmfSender::kDtmfDefaultCommaDelayMs) { std::vector dtmf_queue_ref; - GetDtmfInfoFromString(tones, duration, inter_tone_gap, &dtmf_queue_ref); + GetDtmfInfoFromString(tones, duration, inter_tone_gap, &dtmf_queue_ref, + comma_delay); VerifyOnProvider(dtmf_queue_ref); } @@ -310,15 +315,33 @@ TEST_F(DtmfSenderTest, InsertEmptyTonesToCancelPreviousTask) { VerifyOnObserver("1"); } -TEST_F(DtmfSenderTest, InsertDtmfWithCommaAsDelay) { +TEST_F(DtmfSenderTest, InsertDtmfWithDefaultCommaDelay) { std::string tones = "3,4"; int duration = 100; int inter_tone_gap = 50; + int default_comma_delay = webrtc::DtmfSender::kDtmfDefaultCommaDelayMs; + EXPECT_EQ(dtmf_->comma_delay(), default_comma_delay); EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap)); EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_); VerifyOnProvider(tones, duration, inter_tone_gap); VerifyOnObserver(tones); + EXPECT_EQ(dtmf_->comma_delay(), default_comma_delay); +} + +TEST_F(DtmfSenderTest, InsertDtmfWithNonDefaultCommaDelay) { + std::string tones = "3,4"; + int duration = 100; + int inter_tone_gap = 50; + int default_comma_delay = webrtc::DtmfSender::kDtmfDefaultCommaDelayMs; + int comma_delay = 500; + EXPECT_EQ(dtmf_->comma_delay(), default_comma_delay); + EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap, comma_delay)); + EXPECT_TRUE_SIMULATED_WAIT(observer_->completed(), kMaxWaitMs, fake_clock_); + + VerifyOnProvider(tones, duration, inter_tone_gap, comma_delay); + VerifyOnObserver(tones); + EXPECT_EQ(dtmf_->comma_delay(), comma_delay); } TEST_F(DtmfSenderTest, TryInsertDtmfWhenItDoesNotWork) { @@ -337,6 +360,7 @@ TEST_F(DtmfSenderTest, InsertDtmfWithInvalidDurationOrGap) { EXPECT_FALSE(dtmf_->InsertDtmf(tones, 6001, inter_tone_gap)); EXPECT_FALSE(dtmf_->InsertDtmf(tones, 39, inter_tone_gap)); EXPECT_FALSE(dtmf_->InsertDtmf(tones, duration, 29)); + EXPECT_FALSE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap, 29)); EXPECT_TRUE(dtmf_->InsertDtmf(tones, duration, inter_tone_gap)); }