From 05f71fcb61df680ab0e0dab06ed6578ce062fa21 Mon Sep 17 00:00:00 2001 From: Henrik Lundin Date: Tue, 1 Sep 2015 11:51:58 +0200 Subject: [PATCH] NetEq: Fixing a corner case with depleted sync buffer In some cases, the number of samples (per channel) in NetEq's sync buffer could fall below the allowed minimum (5 samples for narrowband, scaling for other rates). If the number of samples extracted from the buffer was smaller than the desired number, an error is returned. However, if the decoder returns fewer samples than expected, it could happen that the sync buffer level falls under the minimum, but enough samples are extracted. This triggered an assert. With this change, the minimum level of the sync buffer is always enforced. A test is implemented to trigger the problem. It made the assert fire without this fix, but it now passes. BUG=webrtc:4840 R=minyue@webrtc.org Review URL: https://codereview.webrtc.org/1324453002 . Cr-Commit-Position: refs/heads/master@{#9828} --- .../audio_coding_module_unittest_oldapi.cc | 3 + .../neteq/mock/mock_audio_decoder.h | 1 + .../modules/audio_coding/neteq/neteq_impl.cc | 12 +++- .../audio_coding/neteq/neteq_impl_unittest.cc | 63 +++++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc index 4b53a524e9..b7289fc7cb 100644 --- a/webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc +++ b/webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc @@ -1000,6 +1000,9 @@ TEST_F(AcmReceiverBitExactnessOldApi, MAYBE_48kHzOutputExternalDecoder) { EXPECT_CALL(mock_decoder, HasDecodePlc()) .Times(AtLeast(1)) .WillRepeatedly(Invoke(&decoder, &AudioDecoderPcmU::HasDecodePlc)); + EXPECT_CALL(mock_decoder, PacketDuration(_, _)) + .Times(AtLeast(1)) + .WillRepeatedly(Invoke(&decoder, &AudioDecoderPcmU::PacketDuration)); ExternalDecoder ed; ed.rtp_payload_type = 0; ed.external_decoder = &mock_decoder; diff --git a/webrtc/modules/audio_coding/neteq/mock/mock_audio_decoder.h b/webrtc/modules/audio_coding/neteq/mock/mock_audio_decoder.h index 90f132b0f7..8debcbbb1e 100644 --- a/webrtc/modules/audio_coding/neteq/mock/mock_audio_decoder.h +++ b/webrtc/modules/audio_coding/neteq/mock/mock_audio_decoder.h @@ -31,6 +31,7 @@ class MockAudioDecoder : public AudioDecoder { MOCK_METHOD5(IncomingPacket, int(const uint8_t*, size_t, uint16_t, uint32_t, uint32_t)); MOCK_METHOD0(ErrorCode, int()); + MOCK_CONST_METHOD2(PacketDuration, int(const uint8_t*, size_t)); MOCK_CONST_METHOD0(Channels, size_t()); MOCK_CONST_METHOD0(codec_type, NetEqDecoder()); MOCK_METHOD1(CodecSupported, bool(NetEqDecoder)); diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc index beadedadb4..e6f7e60dbd 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -834,6 +834,16 @@ int NetEqImpl::GetAudioInternal(size_t max_length, LOG(LS_VERBOSE) << "Sync buffer (" << *num_channels << " channel(s)):" << " insert " << algorithm_buffer_->Size() << " samples, extract " << samples_from_sync << " samples"; + if (sync_buffer_->FutureLength() < expand_->overlap_length()) { + // The sync buffer should always contain |overlap_length| samples, but now + // too many samples have been extracted. Reinstall the |overlap_length| + // lookahead by moving the index. + const size_t missing_lookahead_samples = + expand_->overlap_length() - sync_buffer_->FutureLength(); + DCHECK_GE(sync_buffer_->next_index(), missing_lookahead_samples); + sync_buffer_->set_next_index(sync_buffer_->next_index() - + missing_lookahead_samples); + } if (samples_from_sync != output_size_samples_) { LOG(LS_ERROR) << "samples_from_sync (" << samples_from_sync << ") != output_size_samples_ (" << output_size_samples_ @@ -846,7 +856,7 @@ int NetEqImpl::GetAudioInternal(size_t max_length, *samples_per_channel = output_size_samples_; // Should always have overlap samples left in the |sync_buffer_|. - assert(sync_buffer_->FutureLength() >= expand_->overlap_length()); + DCHECK_GE(sync_buffer_->FutureLength(), expand_->overlap_length()); if (play_dtmf) { return_value = DtmfOverdub(dtmf_event, sync_buffer_->Channels(), output); diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc index 4f68c66467..3a3469241c 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc @@ -942,4 +942,67 @@ TEST_F(NetEqImplTest, FloodBufferAndGetNetworkStats) { NetEqNetworkStatistics stats; EXPECT_EQ(NetEq::kOK, neteq_->NetworkStatistics(&stats)); } + +TEST_F(NetEqImplTest, DecodedPayloadTooShort) { + UseNoMocks(); + CreateInstance(); + + const uint8_t kPayloadType = 17; // Just an arbitrary number. + const uint32_t kReceiveTime = 17; // Value doesn't matter for this test. + const int kSampleRateHz = 8000; + const size_t kPayloadLengthSamples = + static_cast(10 * kSampleRateHz / 1000); // 10 ms. + const size_t kPayloadLengthBytes = 2 * kPayloadLengthSamples; + uint8_t payload[kPayloadLengthBytes] = {0}; + WebRtcRTPHeader rtp_header; + rtp_header.header.payloadType = kPayloadType; + rtp_header.header.sequenceNumber = 0x1234; + rtp_header.header.timestamp = 0x12345678; + rtp_header.header.ssrc = 0x87654321; + + // Create a mock decoder object. + MockAudioDecoder mock_decoder; + EXPECT_CALL(mock_decoder, Reset()).WillRepeatedly(Return()); + EXPECT_CALL(mock_decoder, Channels()).WillRepeatedly(Return(1)); + EXPECT_CALL(mock_decoder, IncomingPacket(_, kPayloadLengthBytes, _, _, _)) + .WillRepeatedly(Return(0)); + EXPECT_CALL(mock_decoder, PacketDuration(_, _)) + .WillRepeatedly(Return(kPayloadLengthSamples)); + int16_t dummy_output[kPayloadLengthSamples] = {0}; + // The below expectation will make the mock decoder write + // |kPayloadLengthSamples| - 5 zeros to the output array, and mark it as + // speech. That is, the decoded length is 5 samples shorter than the expected. + EXPECT_CALL(mock_decoder, + Decode(_, kPayloadLengthBytes, kSampleRateHz, _, _, _)) + .WillOnce( + DoAll(SetArrayArgument<4>(dummy_output, + dummy_output + kPayloadLengthSamples - 5), + SetArgPointee<5>(AudioDecoder::kSpeech), + Return(kPayloadLengthSamples - 5))); + EXPECT_EQ(NetEq::kOK, + neteq_->RegisterExternalDecoder(&mock_decoder, kDecoderPCM16B, + kPayloadType, kSampleRateHz)); + + // Insert one packet. + EXPECT_EQ(NetEq::kOK, + neteq_->InsertPacket(rtp_header, payload, kPayloadLengthBytes, + kReceiveTime)); + + EXPECT_EQ(5u, neteq_->sync_buffer_for_test()->FutureLength()); + + // Pull audio once. + const size_t kMaxOutputSize = static_cast(10 * kSampleRateHz / 1000); + int16_t output[kMaxOutputSize]; + size_t samples_per_channel; + int num_channels; + NetEqOutputType type; + EXPECT_EQ(NetEq::kOK, + neteq_->GetAudio(kMaxOutputSize, output, &samples_per_channel, + &num_channels, &type)); + ASSERT_EQ(kMaxOutputSize, samples_per_channel); + EXPECT_EQ(1, num_channels); + EXPECT_EQ(kOutputNormal, type); + + EXPECT_CALL(mock_decoder, Die()); +} } // namespace webrtc