From 1118ebac0a455ec253ecd5146365661074593242 Mon Sep 17 00:00:00 2001 From: Andrey Logvin Date: Tue, 10 Aug 2021 18:07:37 +0000 Subject: [PATCH] Revert "red: remove special-casing of no-redundancy" This reverts commit 320c57b7c6bf68d8612a4f135f44b3d29e802113. Reason for revert: Breaks CI tests: https://ci.chromium.org/ui/p/webrtc/builders/ci/Linux32%20Release/27236/overview All CI tests: https://ci.chromium.org/p/webrtc/g/ci/console Error is of the following type: ``` ../../modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc:195: Failure Expected equality of these values: 1u Which is: 1 encoded_info_.encoded_bytes Which is: 2 Stack trace: 0x56b298d9: webrtc::AudioEncoderCopyRedTest_CheckPayloadSizesSingle_Test::TestBody() 0x572fe317: testing::internal::HandleExceptionsInMethodIfSupported<>() 0x572fe1d4: testing::Test::Run() 0x572ff2ee: testing::TestInfo::Run() ``` Original change's description: > red: remove special-casing of no-redundancy > > removes the special-casing of not sending a RED header when there is no redundant payload. > This avoids switching back and forth between the primary and the red payload format (primarily at the start of the connection). > > BUG=webrtc:11640 > > Change-Id: I8e0044bef1ed7c4168d9527645522392db2ed068 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/220932 > Reviewed-by: Jesus de Vicente Pena > Reviewed-by: Henrik Lundin > Commit-Queue: Henrik Lundin > Cr-Commit-Position: refs/heads/master@{#34703} TBR=henrik.lundin@webrtc.org,devicentepena@webrtc.org,minyue@webrtc.org,philipp.hancke@googlemail.com,webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: Ide409232720df32b24022f99228f3b6ae81f06fe No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:11640 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/228421 Reviewed-by: Andrey Logvin Commit-Queue: Andrey Logvin Cr-Commit-Position: refs/heads/master@{#34707} --- .../codecs/red/audio_encoder_copy_red.cc | 19 ++++++++++++++----- .../red/audio_encoder_copy_red_unittest.cc | 11 ++++------- 2 files changed, 18 insertions(+), 12 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 b88dbbf843..07dd3caeb7 100644 --- a/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc +++ b/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc @@ -122,7 +122,12 @@ AudioEncoder::EncodedInfo AudioEncoderCopyRed::EncodeImpl( header_length_bytes += kRedHeaderLength; } - // Allocate room for RFC 2198 header. + // Allocate room for RFC 2198 header if there is redundant data. + // Otherwise this will send the primary payload type without + // wrapping in RED. + if (header_length_bytes == kRedLastHeaderLength) { + header_length_bytes = 0; + } encoded->SetSize(header_length_bytes); // Iterate backwards and append the data. @@ -143,15 +148,17 @@ AudioEncoder::EncodedInfo AudioEncoderCopyRed::EncodeImpl( // `info` will be implicitly cast to an EncodedInfoLeaf struct, effectively // discarding the (empty) vector of redundant information. This is // intentional. - if (header_length_bytes > kRedHeaderLength) { + if (header_length_bytes > 0) { info.redundant.push_back(info); RTC_DCHECK_EQ(info.speech, info.redundant[info.redundant.size() - 1].speech); } encoded->AppendData(primary_encoded_); - RTC_DCHECK_EQ(header_offset, header_length_bytes - 1); - encoded->data()[header_offset] = info.payload_type; + if (header_length_bytes > 0) { + RTC_DCHECK_EQ(header_offset, header_length_bytes - 1); + encoded->data()[header_offset] = info.payload_type; + } // Shift the redundant encodings. auto rit = redundant_encodings_.rbegin(); @@ -165,7 +172,9 @@ AudioEncoder::EncodedInfo AudioEncoderCopyRed::EncodeImpl( it->second.SetData(primary_encoded_); // Update main EncodedInfo. - info.payload_type = red_payload_type_; + if (header_length_bytes > 0) { + info.payload_type = red_payload_type_; + } info.encoded_bytes = encoded->size(); return 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 7a90850b9e..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 @@ -34,8 +34,6 @@ namespace webrtc { namespace { static const size_t kMaxNumSamples = 48 * 10 * 2; // 10 ms @ 48 kHz stereo. -static const size_t kRedLastHeaderLength = - 1; // 1 byte RED header for the last element. } class AudioEncoderCopyRedTest : public ::testing::Test { @@ -156,7 +154,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckNoOutput) { // First call is a special case, since it does not include a secondary // payload. EXPECT_EQ(0u, encoded_info_.redundant.size()); - EXPECT_EQ(kEncodedSize + kRedLastHeaderLength, encoded_info_.encoded_bytes); + EXPECT_EQ(kEncodedSize, encoded_info_.encoded_bytes); // Next call to the speech encoder will not produce any output. Encode(); @@ -219,7 +217,7 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloadSizes) { // payload. Encode(); EXPECT_EQ(0u, encoded_info_.redundant.size()); - EXPECT_EQ(kRedLastHeaderLength + 1u, encoded_info_.encoded_bytes); + EXPECT_EQ(1u, encoded_info_.encoded_bytes); // Second call is also special since it does not include a tertiary // payload. @@ -331,10 +329,9 @@ TEST_F(AudioEncoderCopyRedTest, CheckPayloads) { // First call is a special case, since it does not include a secondary // payload. Encode(); - EXPECT_EQ(kRedLastHeaderLength + kPayloadLenBytes, - encoded_info_.encoded_bytes); + EXPECT_EQ(kPayloadLenBytes, encoded_info_.encoded_bytes); for (size_t i = 0; i < kPayloadLenBytes; ++i) { - EXPECT_EQ(i, encoded_.data()[kRedLastHeaderLength + i]); + EXPECT_EQ(i, encoded_.data()[i]); } for (int j = 0; j < 1; ++j) {