Refactor WVoE DTMF handling #2

talk/media/webrtc/webrtcvoiceengine.cc

 /*
  * libjingle
  * Copyright 2004 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 115 matching lines @@
 // NOTE(ajm): Don't use hardcoded paths on platforms not explicitly specified
 // below.
 #if defined(CHROMEOS)
 const char kAecDumpByAudioOptionFilename[] = "/tmp/audio.aecdump";
 #elif defined(ANDROID)
 const char kAecDumpByAudioOptionFilename[] = "/sdcard/audio.aecdump";
 #else
 const char kAecDumpByAudioOptionFilename[] = "audio.aecdump";
 #endif
 
+// Constants from voice_engine_defines.h.
+const int kMinTelephoneEventCode = 0;           // RFC4733 (Section 2.3.1)
+const int kMaxTelephoneEventCode = 255;
+const int kMinTelephoneEventDuration = 100;
+const int kMaxTelephoneEventDuration = 60000;   // Actual limit is 2^16
+
 bool ValidateStreamParams(const StreamParams& sp) {
   if (sp.ssrcs.empty()) {
     LOG(LS_ERROR) << "No SSRCs in stream parameters: " << sp.ToString();
     return false;
   }
   if (sp.ssrcs.size() > 1) {
     LOG(LS_ERROR) << "Multiple SSRCs in stream parameters: " << sp.ToString();
     return false;
   }
   return true;
@@ skipping 442 matching lines @@
   if (!SetOptions(GetDefaultEngineOptions())) {
     return false;
   }
 
   // Print our codec list again for the call diagnostic log
   LOG(LS_INFO) << "WebRtc VoiceEngine codecs:";
   for (const AudioCodec& codec : codecs_) {
     LOG(LS_INFO) << ToString(codec);
   }
 
-  // Disable the DTMF playout when a tone is sent.
-  // PlayDtmfTone will be used if local playout is needed.
-  if (voe_wrapper_->dtmf()->SetDtmfFeedbackStatus(false) == -1) {
-    LOG_RTCERR1(SetDtmfFeedbackStatus, false);
-  }
-
   initialized_ = true;
   return true;
 }
 
 void WebRtcVoiceEngine::Terminate() {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "WebRtcVoiceEngine::Terminate";
   initialized_ = false;
 
   StopAecDump();
@@ skipping 709 matching lines @@
 
   // Callback from the |renderer_| when it is going away. In case Start() has
   // never been called, this callback won't be triggered.
   void OnClose() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     // Set |renderer_| to nullptr to make sure no more callback will get into
     // the renderer.
     renderer_ = nullptr;
   }
 
+  // Accessor to the Call stream.
+  webrtc::AudioSendStream* stream() {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    RTC_DCHECK(stream_);
+    return stream_;

[pthatcher1, 2015/12/02 18:45:37]

Rather than expose the stream underneath, it seems like we should just add a
SendTelephoneEvent method that calls the SendTelephoneEvent method on the
stream_.

[the sun, 2015/12/03 09:57:47]

Done.

+  }
+
   // Accessor to the VoE channel ID.
   int channel() const {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     return config_.voe_channel_id;
   }
 
  private:
   rtc::ThreadChecker worker_thread_checker_;
   rtc::ThreadChecker audio_capture_thread_checker_;
   webrtc::AudioTransport* const voe_audio_transport_ = nullptr;
@@ skipping 263 matching lines @@
 
 bool WebRtcVoiceMediaChannel::SetSendCodecs(
     int channel, const std::vector<AudioCodec>& codecs) {
   // Disable VAD, FEC, and RED unless we know the other side wants them.
   engine()->voe()->codec()->SetVADStatus(channel, false);
   engine()->voe()->rtp()->SetNACKStatus(channel, false, 0);
   engine()->voe()->rtp()->SetREDStatus(channel, false);
   engine()->voe()->codec()->SetFECStatus(channel, false);
 
   // Scan through the list to figure out the codec to use for sending, along
-  // with the proper configuration for VAD and DTMF.
+  // with the proper configuration for VAD.
   bool found_send_codec = false;
   webrtc::CodecInst send_codec;
   memset(&send_codec, 0, sizeof(send_codec));
 
   bool nack_enabled = nack_enabled_;
   bool enable_codec_fec = false;
   bool enable_opus_dtx = false;
   int opus_max_playback_rate = 0;
 
   // Set send codec (the first non-telephone-event/CN codec)
@@ skipping 108 matching lines @@
     }
   }
 
   // Always update the |send_codec_| to the currently set send codec.
   send_codec_.reset(new webrtc::CodecInst(send_codec));
 
   if (send_bitrate_setting_) {
     SetSendBitrateInternal(send_bitrate_bps_);
   }
 
-  // Loop through the codecs list again to config the telephone-event/CN codec.
+  // Loop through the codecs list again to config the CN codec.
   for (const AudioCodec& codec : codecs) {
     // Ignore codecs we don't know about. The negotiation step should prevent
     // this, but double-check to be sure.
     webrtc::CodecInst voe_codec;
     if (!WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {
       LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
       continue;
     }
 
-    // Find the DTMF telephone event "codec" and tell VoiceEngine channels
-    // about it.
-    if (IsCodec(codec, kDtmfCodecName)) {
-      if (engine()->voe()->dtmf()->SetSendTelephoneEventPayloadType(
-              channel, codec.id) == -1) {
-        LOG_RTCERR2(SetSendTelephoneEventPayloadType, channel, codec.id);
-        return false;
-      }
-    } else if (IsCodec(codec, kCnCodecName)) {
+    if (IsCodec(codec, kCnCodecName)) {
       // Turn voice activity detection/comfort noise on if supported.
       // Set the wideband CN payload type appropriately.
       // (narrowband always uses the static payload type 13).
       webrtc::PayloadFrequencies cn_freq;
       switch (codec.clockrate) {
         case 8000:
           cn_freq = webrtc::kFreq8000Hz;
           break;
         case 16000:
           cn_freq = webrtc::kFreq16000Hz;
@@ skipping 34 matching lines @@
         }
       }
     }
   }
   return true;
 }
 
 bool WebRtcVoiceMediaChannel::SetSendCodecs(
     const std::vector<AudioCodec>& codecs) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+  // TODO(solenberg): Validate input - that payload types don't overlap, are
+  //                  within range, filter out codecs we don't support,
+  //                  redundant codecs etc.
 
-  dtmf_allowed_ = false;
+  // Find the DTMF telephone event "codec" payload type.
+  dtmf_payload_type_ = rtc::Optional<int>();
   for (const AudioCodec& codec : codecs) {
-    // Find the DTMF telephone event "codec".
     if (IsCodec(codec, kDtmfCodecName)) {
-      dtmf_allowed_ = true;
+      dtmf_payload_type_ = rtc::Optional<int>(codec.id);
+      break;
     }
   }
 
   // Cache the codecs in order to configure the channel created later.
   send_codecs_ = codecs;
   for (const auto& ch : send_streams_) {
     if (!SetSendCodecs(ch.second->channel(), codecs)) {
       return false;
     }
   }
@@ skipping 442 matching lines @@
                                                                      volume)) {
     LOG_RTCERR2(SetChannelOutputVolumeScaling, ch_id, volume);
     return false;
   }
   LOG(LS_INFO) << "SetOutputVolume to " << volume
                << " for channel " << ch_id << " and ssrc " << ssrc;
   return true;
 }
 
 bool WebRtcVoiceMediaChannel::CanInsertDtmf() {
-  return dtmf_allowed_;
+  return dtmf_payload_type_ ? true : false;
 }
 
 bool WebRtcVoiceMediaChannel::InsertDtmf(uint32_t ssrc, int event,
                                          int duration) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-  if (!dtmf_allowed_) {
+  LOG(LS_INFO) << "WebRtcVoiceMediaChannel::InsertDtmf";
+  if (!dtmf_payload_type_) {
     return false;
   }
 
-  // Send the event.
-  int channel = -1;
+  // Figure out which WebRtcAudioSendStream to send the event on.
+  auto it = send_streams_.end();
   if (ssrc == 0) {
-    if (send_streams_.size() > 0) {
-      channel = send_streams_.begin()->second->channel();
-    }
+    it = send_streams_.begin();
   } else {
-    channel = GetSendChannelId(ssrc);
+    it = send_streams_.find(ssrc);
   }

[pthatcher1, 2015/12/02 18:45:37]

Does this work?

audio it = ssrc ? send_streams_.find(ssrc) : send_streams_.begin();

[the sun, 2015/12/03 09:57:47]

Nice one! 
Except "audio" is not a recognized C++ keyword. ;)

-  if (channel == -1) {
-    LOG(LS_WARNING) << "InsertDtmf - The specified ssrc "
-                    << ssrc << " is not in use.";
+  if (it == send_streams_.end()) {
+    LOG(LS_WARNING) << "The specified ssrc " << ssrc << " is not in use.";
     return false;
   }
-  // Send DTMF using out-of-band DTMF. ("true", as 3rd arg)
-  if (engine()->voe()->dtmf()->SendTelephoneEvent(
-          channel, event, true, duration) == -1) {
-    LOG_RTCERR4(SendTelephoneEvent, channel, event, true, duration);
+  if (event < kMinTelephoneEventCode ||
+      event > kMaxTelephoneEventCode) {
+    LOG(LS_WARNING) << "DTMF event code " << event << " out of range.";
     return false;
   }
-
-  return true;
+  if (duration < kMinTelephoneEventDuration ||
+      duration > kMaxTelephoneEventDuration) {
+    LOG(LS_WARNING) << "DTMF event duration " << duration << " out of range.";
+    return false;
+  }
+  return it->second->stream()->SendTelephoneEvent(*dtmf_payload_type_, event,
+                                                  duration);
 }
 
 void WebRtcVoiceMediaChannel::OnPacketReceived(
     rtc::Buffer* packet, const rtc::PacketTime& packet_time) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
 
   uint32_t ssrc = 0;
   if (!GetRtpSsrc(packet->data(), packet->size(), &ssrc)) {
     return;
   }
@@ skipping 311 matching lines @@
     }
   } else {
     LOG(LS_INFO) << "Stopping playout for channel #" << channel;
     engine()->voe()->base()->StopPlayout(channel);
   }
   return true;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE