From 4b64411406af7b35e330708b2eddf82d468cf32b Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Fri, 11 Oct 2019 09:37:42 +0200 Subject: [PATCH] NetEqImpl::GetDecoderFormat: Return RTP clockrate, not codec sample rate Well, in fact we need to return both. But return codec sample rate separately and let the SdpAudioFormat contain the RTP clockrate, otherwise we're essentially lying to our callers. Bug: webrtc:11028 Change-Id: I40f36cb9db6b9824404ade6b0515a8312ff97009 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/156307 Commit-Queue: Karl Wiberg Reviewed-by: Ivo Creusen Cr-Commit-Position: refs/heads/master@{#29444} --- audio/channel_receive.cc | 3 +++ modules/audio_coding/acm2/acm_receiver.cc | 18 ++++++++++-------- modules/audio_coding/acm2/acm_receiver.h | 10 ++++++++-- modules/audio_coding/neteq/include/neteq.h | 9 ++++++++- modules/audio_coding/neteq/neteq_impl.cc | 22 ++++++++++++---------- modules/audio_coding/neteq/neteq_impl.h | 2 +- 6 files changed, 42 insertions(+), 22 deletions(-) diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 5bb568e4cf..486dcb11ac 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -888,6 +888,9 @@ int ChannelReceive::GetRtpTimestampRateHz() const { // TODO(ossu): Zero clockrate can only happen if we've added an external // decoder for a format we don't support internally. Remove once that way of // adding decoders is gone! + // TODO(kwiberg): `decoder->second.clockrate_hz` is an RTP clockrate as it + // should, but `acm_receiver_.last_output_sample_rate_hz()` is a codec sample + // rate, which is not always the same thing. return (decoder && decoder->second.clockrate_hz != 0) ? decoder->second.clockrate_hz : acm_receiver_.last_output_sample_rate_hz(); diff --git a/modules/audio_coding/acm2/acm_receiver.cc b/modules/audio_coding/acm2/acm_receiver.cc index 1c8d88da6e..40196151af 100644 --- a/modules/audio_coding/acm2/acm_receiver.cc +++ b/modules/audio_coding/acm2/acm_receiver.cc @@ -73,7 +73,7 @@ absl::optional AcmReceiver::last_packet_sample_rate_hz() const { if (!last_decoder_) { return absl::nullopt; } - return last_decoder_->second.clockrate_hz; + return last_decoder_->sample_rate_hz; } int AcmReceiver::last_output_sample_rate_hz() const { @@ -89,7 +89,7 @@ int AcmReceiver::InsertPacket(const RTPHeader& rtp_header, int payload_type = rtp_header.payloadType; auto format = neteq_->GetDecoderFormat(payload_type); - if (format && absl::EqualsIgnoreCase(format->name, "red")) { + if (format && absl::EqualsIgnoreCase(format->sdp_format.name, "red")) { // This is a RED packet. Get the format of the audio codec. payload_type = incoming_payload[0] & 0x7f; format = neteq_->GetDecoderFormat(payload_type); @@ -102,15 +102,17 @@ int AcmReceiver::InsertPacket(const RTPHeader& rtp_header, { rtc::CritScope lock(&crit_sect_); - if (absl::EqualsIgnoreCase(format->name, "cn")) { - if (last_decoder_ && last_decoder_->second.num_channels > 1) { + if (absl::EqualsIgnoreCase(format->sdp_format.name, "cn")) { + if (last_decoder_ && last_decoder_->num_channels > 1) { // This is a CNG and the audio codec is not mono, so skip pushing in // packets into NetEq. return 0; } } else { - RTC_DCHECK(format); - last_decoder_ = std::make_pair(payload_type, *format); + last_decoder_ = DecoderInfo{/*payload_type=*/payload_type, + /*sample_rate_hz=*/format->sample_rate_hz, + /*num_channels=*/format->num_channels, + /*sdp_format=*/std::move(format->sdp_format)}; } } // |crit_sect_| is released. @@ -221,8 +223,8 @@ absl::optional> AcmReceiver::LastDecoder() if (!last_decoder_) { return absl::nullopt; } - RTC_DCHECK_NE(-1, last_decoder_->first); // Payload type should be valid. - return last_decoder_; + RTC_DCHECK_NE(-1, last_decoder_->payload_type); + return std::make_pair(last_decoder_->payload_type, last_decoder_->sdp_format); } void AcmReceiver::GetNetworkStatistics(NetworkStatistics* acm_stat) const { diff --git a/modules/audio_coding/acm2/acm_receiver.h b/modules/audio_coding/acm2/acm_receiver.h index f07f8a951c..15126566ae 100644 --- a/modules/audio_coding/acm2/acm_receiver.h +++ b/modules/audio_coding/acm2/acm_receiver.h @@ -203,11 +203,17 @@ class AcmReceiver { void GetDecodingCallStatistics(AudioDecodingCallStats* stats) const; private: + struct DecoderInfo { + int payload_type; + int sample_rate_hz; + int num_channels; + SdpAudioFormat sdp_format; + }; + uint32_t NowInTimestamp(int decoder_sampling_rate) const; rtc::CriticalSection crit_sect_; - absl::optional> last_decoder_ - RTC_GUARDED_BY(crit_sect_); + absl::optional last_decoder_ RTC_GUARDED_BY(crit_sect_); ACMResampler resampler_ RTC_GUARDED_BY(crit_sect_); std::unique_ptr last_audio_buffer_ RTC_GUARDED_BY(crit_sect_); CallStatistics call_stats_ RTC_GUARDED_BY(crit_sect_); diff --git a/modules/audio_coding/neteq/include/neteq.h b/modules/audio_coding/neteq/include/neteq.h index c6af751757..b53b5ada9f 100644 --- a/modules/audio_coding/neteq/include/neteq.h +++ b/modules/audio_coding/neteq/include/neteq.h @@ -143,6 +143,13 @@ class NetEq { enum ReturnCodes { kOK = 0, kFail = -1 }; + // Return type for GetDecoderFormat. + struct DecoderFormat { + int sample_rate_hz; + int num_channels; + SdpAudioFormat sdp_format; + }; + // Creates a new NetEq object, with parameters set in |config|. The |config| // object will only have to be valid for the duration of the call to this // method. @@ -265,7 +272,7 @@ class NetEq { // Returns the decoder info for the given payload type. Returns empty if no // such payload type was registered. - virtual absl::optional GetDecoderFormat( + virtual absl::optional GetDecoderFormat( int payload_type) const = 0; // Flushes both the packet buffer and the sync buffer. diff --git a/modules/audio_coding/neteq/neteq_impl.cc b/modules/audio_coding/neteq/neteq_impl.cc index 751fc457de..37036e34c4 100644 --- a/modules/audio_coding/neteq/neteq_impl.cc +++ b/modules/audio_coding/neteq/neteq_impl.cc @@ -392,21 +392,23 @@ int NetEqImpl::last_output_sample_rate_hz() const { return last_output_sample_rate_hz_; } -absl::optional NetEqImpl::GetDecoderFormat( +absl::optional NetEqImpl::GetDecoderFormat( int payload_type) const { rtc::CritScope lock(&crit_sect_); const DecoderDatabase::DecoderInfo* const di = decoder_database_->GetDecoderInfo(payload_type); - if (!di) { - return absl::nullopt; // Payload type not registered. + if (di) { + const AudioDecoder* const decoder = di->GetDecoder(); + // TODO(kwiberg): Why the special case for RED? + return DecoderFormat{ + /*sample_rate_hz=*/di->IsRed() ? 8000 : di->SampleRateHz(), + /*num_channels=*/ + decoder ? rtc::dchecked_cast(decoder->Channels()) : 1, + /*sdp_format=*/di->GetFormat()}; + } else { + // Payload type not registered. + return absl::nullopt; } - - SdpAudioFormat format = di->GetFormat(); - // TODO(solenberg): This is legacy but messed up - mixing RTP rate and SR. - format.clockrate_hz = di->IsRed() ? 8000 : di->SampleRateHz(); - const AudioDecoder* const decoder = di->GetDecoder(); - format.num_channels = decoder ? decoder->Channels() : 1; - return format; } void NetEqImpl::FlushBuffers() { diff --git a/modules/audio_coding/neteq/neteq_impl.h b/modules/audio_coding/neteq/neteq_impl.h index 8ecb9b6f74..842869f9fc 100644 --- a/modules/audio_coding/neteq/neteq_impl.h +++ b/modules/audio_coding/neteq/neteq_impl.h @@ -182,7 +182,7 @@ class NetEqImpl : public webrtc::NetEq { int last_output_sample_rate_hz() const override; - absl::optional GetDecoderFormat( + absl::optional GetDecoderFormat( int payload_type) const override; // Flushes both the packet buffer and the sync buffer.