From 43f25e36f79b09224b31bf41eeb033ad0916ee24 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 10 Aug 2021 17:21:25 +0200 Subject: [PATCH] red: fix redundancy shift and add tests fixes an incorrect redundancy shift and add tests that would have caught this bug. BUG=webrtc:11640 Change-Id: I6fe2fb21587fffc5fee4d403ac898e12d525a1cd Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/224120 Reviewed-by: Henrik Lundin Commit-Queue: Henrik Lundin Cr-Commit-Position: refs/heads/master@{#34702} --- .../codecs/red/audio_encoder_copy_red.cc | 10 +-- .../red/audio_encoder_copy_red_unittest.cc | 88 ++++++++++++++++++- 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc b/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc index 1ca7a842c1..07dd3caeb7 100644 --- a/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc +++ b/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc @@ -161,11 +161,11 @@ AudioEncoder::EncodedInfo AudioEncoderCopyRed::EncodeImpl( } // Shift the redundant encodings. - it = redundant_encodings_.begin(); - for (auto next = std::next(it); next != redundant_encodings_.end(); - it++, next = std::next(it)) { - next->first = it->first; - next->second.SetData(it->second); + auto rit = redundant_encodings_.rbegin(); + for (auto next = std::next(rit); next != redundant_encodings_.rend(); + rit++, next = std::next(rit)) { + rit->first = next->first; + rit->second.SetData(next->second); } it = redundant_encodings_.begin(); it->first = info; diff --git a/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc b/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc index ddd82441db..06404ab5b6 100644 --- a/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc +++ b/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc @@ -15,6 +15,7 @@ #include "rtc_base/checks.h" #include "rtc_base/numerics/safe_conversions.h" +#include "test/field_trial.h" #include "test/gtest.h" #include "test/mock_audio_encoder.h" #include "test/testsupport/rtc_expect_death.h" @@ -165,6 +166,41 @@ TEST_F(AudioEncoderCopyRedTest, CheckNoOutput) { ASSERT_EQ(2u, encoded_info_.redundant.size()); } +// Checks that the correct payload sizes are populated into the redundancy +// information. Variant with a redundancy level of 1. +TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizesSingle) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-Audio-Red-For-Opus/Enabled-1/"); + // Recreate the RED encoder to take the new field trial setting into account. + AudioEncoderCopyRed::Config config; + config.payload_type = red_payload_type_; + config.speech_encoder = std::move(red_->ReclaimContainedEncoders()[0]); + red_.reset(new AudioEncoderCopyRed(std::move(config))); + + // Let the mock encoder return payload sizes 1, 2, 3, ..., 10 for the sequence + // of calls. + static const int kNumPackets = 10; + InSequence s; + for (int encode_size = 1; encode_size <= kNumPackets; ++encode_size) { + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) + .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(encode_size))); + } + + // First call is a special case, since it does not include a secondary + // payload. + Encode(); + EXPECT_EQ(0u, encoded_info_.redundant.size()); + EXPECT_EQ(1u, encoded_info_.encoded_bytes); + + for (size_t i = 2; i <= kNumPackets; ++i) { + Encode(); + ASSERT_EQ(2u, encoded_info_.redundant.size()); + EXPECT_EQ(i, encoded_info_.redundant[1].encoded_bytes); + EXPECT_EQ(i - 1, encoded_info_.redundant[0].encoded_bytes); + EXPECT_EQ(5 + i + (i - 1), encoded_info_.encoded_bytes); + } +} + // Checks that the correct payload sizes are populated into the redundancy // information. TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizes) { @@ -183,7 +219,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizes) { EXPECT_EQ(0u, encoded_info_.redundant.size()); EXPECT_EQ(1u, encoded_info_.encoded_bytes); - // Second call is also special since it does not include a ternary + // Second call is also special since it does not include a tertiary // payload. Encode(); EXPECT_EQ(2u, encoded_info_.redundant.size()); @@ -199,6 +235,56 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizes) { } } +// Checks that the correct payload sizes are populated into the redundancy +// information. Variant with a redundancy level of 3. +TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizesTriple) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-Audio-Red-For-Opus/Enabled-3/"); + // Recreate the RED encoder to take the new field trial setting into account. + AudioEncoderCopyRed::Config config; + config.payload_type = red_payload_type_; + config.speech_encoder = std::move(red_->ReclaimContainedEncoders()[0]); + red_.reset(new AudioEncoderCopyRed(std::move(config))); + + // Let the mock encoder return payload sizes 1, 2, 3, ..., 10 for the sequence + // of calls. + static const int kNumPackets = 10; + InSequence s; + for (int encode_size = 1; encode_size <= kNumPackets; ++encode_size) { + EXPECT_CALL(*mock_encoder_, EncodeImpl(_, _, _)) + .WillOnce(Invoke(MockAudioEncoder::FakeEncoding(encode_size))); + } + + // First call is a special case, since it does not include a secondary + // payload. + Encode(); + EXPECT_EQ(0u, encoded_info_.redundant.size()); + EXPECT_EQ(1u, encoded_info_.encoded_bytes); + + // Second call is also special since it does not include a tertiary + // payload. + Encode(); + EXPECT_EQ(2u, encoded_info_.redundant.size()); + EXPECT_EQ(8u, encoded_info_.encoded_bytes); + + // Third call is also special since it does not include a quaternary + // payload. + Encode(); + EXPECT_EQ(3u, encoded_info_.redundant.size()); + EXPECT_EQ(15u, encoded_info_.encoded_bytes); + + for (size_t i = 4; i <= kNumPackets; ++i) { + Encode(); + ASSERT_EQ(4u, encoded_info_.redundant.size()); + EXPECT_EQ(i, encoded_info_.redundant[3].encoded_bytes); + EXPECT_EQ(i - 1, encoded_info_.redundant[2].encoded_bytes); + EXPECT_EQ(i - 2, encoded_info_.redundant[1].encoded_bytes); + EXPECT_EQ(i - 3, encoded_info_.redundant[0].encoded_bytes); + EXPECT_EQ(13 + i + (i - 1) + (i - 2) + (i - 3), + encoded_info_.encoded_bytes); + } +} + // Checks that the correct timestamps are returned. TEST_F(AudioEncoderCopyRedTest, CheckTimestamps) { uint32_t primary_timestamp = timestamp_;