From 00bceb1eda186848a7883b43d024d42a67d724b5 Mon Sep 17 00:00:00 2001 From: ossu Date: Fri, 2 Dec 2016 02:40:02 -0800 Subject: [PATCH] Deprecated SetAudioPacketSize from RTPSender and removed calls to it. The packet size was only used to control how often to output DTMF packets. However, it likely did not work as intended, since that interval was only set during initialization. No changes to the packet size, like what AudioEncoder::Num10MsFramesInNextPacket could indicate, were picked up. The value was instead taken from an entry in ACMCodecDB. Since it was not-fully-functional, its exact value didn't seem to matter and it was getting in the way of making it possible to supply an external audio encoder factory, I've decided to remove it altogether. The DTMF code now uses an interval of 50 ms regardless, which is a value recommended by the RFC. BUG=webrtc:5806 Review-Url: https://codereview.webrtc.org/2545753002 Cr-Commit-Position: refs/heads/master@{#15380} --- webrtc/modules/rtp_rtcp/include/rtp_rtcp.h | 7 +++--- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 4 +--- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 4 ++-- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 2 +- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 6 ++--- .../rtp_rtcp/source/rtp_sender_audio.cc | 24 +++++++++---------- .../rtp_rtcp/source/rtp_sender_audio.h | 6 ----- webrtc/voice_engine/channel.cc | 6 ----- 8 files changed, 23 insertions(+), 36 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h index b0f50755dd..94853d2f6b 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h @@ -430,10 +430,11 @@ class RtpRtcp : public Module { // Audio // ************************************************************************** - // Sets audio packet size, used to determine when it's time to send a DTMF - // packet in silence (CNG). + // This function is deprecated. It was previously used to determine when it + // was time to send a DTMF packet in silence (CNG). // Returns -1 on failure else 0. - virtual int32_t SetAudioPacketSize(uint16_t packet_size_samples) = 0; + RTC_DEPRECATED virtual int32_t SetAudioPacketSize( + uint16_t packet_size_samples) = 0; // Sends a TelephoneEvent tone using RFC 2833 (4733). // Returns -1 on failure else 0. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index b80c8daf8b..b7b22d900b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -777,11 +777,9 @@ int32_t ModuleRtpRtcpImpl::SendTelephoneEventOutband( return rtp_sender_.SendTelephoneEvent(key, time_ms, level); } -// Set audio packet size, used to determine when it's time to send a DTMF -// packet in silence (CNG). int32_t ModuleRtpRtcpImpl::SetAudioPacketSize( const uint16_t packet_size_samples) { - return rtp_sender_.SetAudioPacketSize(packet_size_samples); + return audio_ ? 0 : -1; } int32_t ModuleRtpRtcpImpl::SetAudioLevel( diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index bd978d05b5..9363a4716a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -260,8 +260,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { // Audio part. - // Set audio packet size, used to determine when it's time to send a DTMF - // packet in silence (CNG). + // This function is deprecated. It was previously used to determine when it + // was time to send a DTMF packet in silence (CNG). int32_t SetAudioPacketSize(uint16_t packet_size_samples) override; // Send a TelephoneEvent tone using RFC 2833 (4733). diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 1b6df005d6..6dcff3ab37 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -1153,7 +1153,7 @@ int32_t RTPSender::SetAudioPacketSize(uint16_t packet_size_samples) { if (!audio_configured_) { return -1; } - return audio_->SetAudioPacketSize(packet_size_samples); + return 0; } int32_t RTPSender::SetAudioLevel(uint8_t level_d_bov) { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index 66db7502e9..080165a0bd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -179,9 +179,9 @@ class RTPSender { // Send a DTMF tone using RFC 2833 (4733). int32_t SendTelephoneEvent(uint8_t key, uint16_t time_ms, uint8_t level); - // Set audio packet size, used to determine when it's time to send a DTMF - // packet in silence (CNG). - int32_t SetAudioPacketSize(uint16_t packet_size_samples); + // This function is deprecated. It was previously used to determine when it + // was time to send a DTMF packet in silence (CNG). + RTC_DEPRECATED int32_t SetAudioPacketSize(uint16_t packet_size_samples); // Store the audio level in d_bov for // header-extension-for-audio-level-indication. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc index 69eb208f10..0f32d4e786 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc @@ -31,14 +31,6 @@ RTPSenderAudio::RTPSenderAudio(Clock* clock, RTPSender* rtp_sender) RTPSenderAudio::~RTPSenderAudio() {} -// set audio packet size, used to determine when it's time to send a DTMF packet -// in silence (CNG) -int32_t RTPSenderAudio::SetAudioPacketSize(uint16_t packet_size_samples) { - rtc::CritScope cs(&send_audio_critsect_); - packet_size_samples_ = packet_size_samples; - return 0; -} - int32_t RTPSenderAudio::RegisterAudioPayload( const char payloadName[RTP_PAYLOAD_NAME_SIZE], const int8_t payload_type, @@ -132,19 +124,24 @@ bool RTPSenderAudio::SendAudio(FrameType frame_type, const uint8_t* payload_data, size_t payload_size, const RTPFragmentationHeader* fragmentation) { + // From RFC 4733: + // A source has wide latitude as to how often it sends event updates. A + // natural interval is the spacing between non-event audio packets. [...] + // Alternatively, a source MAY decide to use a different spacing for event + // updates, with a value of 50 ms RECOMMENDED. + constexpr int kDtmfIntervalTimeMs = 50; uint8_t audio_level_dbov = 0; - uint16_t packet_size_samples = 0; uint32_t dtmf_payload_freq = 0; { rtc::CritScope cs(&send_audio_critsect_); audio_level_dbov = audio_level_dbov_; - packet_size_samples = packet_size_samples_; dtmf_payload_freq = dtmf_payload_freq_; } // Check if we have pending DTMFs to send if (!dtmf_event_is_on_ && dtmf_queue_.PendingDtmf()) { - if ((clock_->TimeInMilliseconds() - dtmf_time_last_sent_) > 50) { + if ((clock_->TimeInMilliseconds() - dtmf_time_last_sent_) > + kDtmfIntervalTimeMs) { // New tone to play dtmf_timestamp_ = rtp_timestamp; if (dtmf_queue_.NextDtmf(&dtmf_current_event_)) { @@ -163,7 +160,10 @@ bool RTPSenderAudio::SendAudio(FrameType frame_type, // kEmptyFrame is used to drive the DTMF when in CN mode // it can be triggered more frequently than we want to send the // DTMF packets. - if (packet_size_samples > (rtp_timestamp - dtmf_timestamp_last_sent_)) { + const unsigned int dtmf_interval_time_rtp = + dtmf_payload_freq * kDtmfIntervalTimeMs / 1000; + if ((rtp_timestamp - dtmf_timestamp_last_sent_) < + dtmf_interval_time_rtp) { // not time to send yet return true; } diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h b/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h index ee4831c634..cf79120bb9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_audio.h @@ -42,10 +42,6 @@ class RTPSenderAudio { size_t payload_size, const RTPFragmentationHeader* fragmentation); - // set audio packet size, used to determine when it's time to send a DTMF - // packet in silence (CNG) - int32_t SetAudioPacketSize(uint16_t packet_size_samples); - // Store the audio level in dBov for // header-extension-for-audio-level-indication. // Valid range is [0,100]. Actual value is negative. @@ -69,8 +65,6 @@ class RTPSenderAudio { rtc::CriticalSection send_audio_critsect_; - uint16_t packet_size_samples_ GUARDED_BY(send_audio_critsect_) = 160; - // DTMF. bool dtmf_event_is_on_ = false; bool dtmf_event_first_packet_sent_ = false; diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index 6e2e91c0a1..8d95c7bfbb 100644 --- a/webrtc/voice_engine/channel.cc +++ b/webrtc/voice_engine/channel.cc @@ -1323,12 +1323,6 @@ int32_t Channel::SetSendCodec(const CodecInst& codec) { } } - if (_rtpRtcpModule->SetAudioPacketSize(codec.pacsize) != 0) { - WEBRTC_TRACE(kTraceError, kTraceVoice, VoEId(_instanceId, _channelId), - "SetSendCodec() failed to set audio packet size"); - return -1; - } - return 0; }