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 <steveanton@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30354}

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;
 };

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 kDtmfCommaDelay = -1;
-static const int kDtmfTwoSecondInMs = 2000;
 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<void>(
@@ -180,10 +186,12 @@ void DtmfSender::DoInsertDtmf() {
   }
 
   int tone_gap = inter_tone_gap_;
-  if (code == kDtmfCodeTwoSecondDelay) {
+  if (code == kDtmfCommaDelay) {
-    // Special case defined by WebRTC - The character',' indicates a delay of 2
+    // Special case defined by WebRTC - By default, the character ',' indicates
-    // seconds before processing the next character in the tones parameter.
+    // a delay of 2 seconds before processing the next character in the tones
-    tone_gap = kDtmfTwoSecondInMs;
+    // 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.";

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.

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,
+  void GetDtmfInfoFromString(
-                             int duration,
+      const std::string& tones,
-                             int inter_tone_gap,
+      int duration,
-                             std::vector<FakeDtmfProvider::DtmfInfo>* dtmfs) {
+      int inter_tone_gap,
+      std::vector<FakeDtmfProvider::DtmfInfo>* 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,
+  void VerifyOnProvider(
-                        int duration,
+      const std::string& tones,
-                        int inter_tone_gap) {
+      int duration,
+      int inter_tone_gap,
+      int comma_delay = webrtc::DtmfSender::kDtmfDefaultCommaDelayMs) {
     std::vector<FakeDtmfProvider::DtmfInfo> 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));
 }