Revert "Reland "Add plumbing of RtpPacketInfos to each AudioFrame as input for SourceTracker.""
This reverts commit 9973933d2e606d64fcdc753acb9ba3afd6e30569.
Reason for revert: breaking downstream projects and not reviewed by direct owners
Original change's description:
> Reland "Add plumbing of RtpPacketInfos to each AudioFrame as input for SourceTracker."
>
> This reverts commit 24192c267a40eb7d6b1850489ccdbf7a84f8ff0f.
>
> Reason for revert: Analyzed the performance regression in more detail.
>
> Most of the regression comes from the extra RtpPacketInfos-related memory allocations in every `NetEq::GetAudio()` call. Commit 1796a820f60cb9429bf4bcf13a40a41794ac8fb0 has removed roughly 2/3rds of the extra allocations from the impacted perf tests. Remaining perf impact is expected to be about "8 microseconds of CPU time per second" on the Linux benchmarking machines and "15 us per second" on Windows/Mac.
>
> There are options to optimize further but they are unlikely worth doing. Note for example that `NetEqPerformanceTest` uses the PCM codec while the real-world use cases would likely use the much heavier Opus codec. The numbers from `OpusSpeedTest` and `NetEqPerformanceTest` suggest that Opus decoding is about 10x as expensive as NetEq overall.
>
> Original change's description:
> > Revert "Add plumbing of RtpPacketInfos to each AudioFrame as input for SourceTracker."
> >
> > This reverts commit 3e8ef940fe86cf6285afb80e68d2a0bedc631b9f.
> >
> > Reason for revert: This CL causes a performance regression in NetEq, see https://bugs.chromium.org/p/chromium/issues/detail?id=982260.
> >
> > Original change's description:
> > > Add plumbing of RtpPacketInfos to each AudioFrame as input for SourceTracker.
> > >
> > > This change adds the plumbing of RtpPacketInfo from ChannelReceive::OnRtpPacket() to ChannelReceive::GetAudioFrameWithInfo() for audio. It is a step towards replacing the non-spec compliant ContributingSources that updates itself at packet-receive time, with the spec-compliant SourceTracker that will update itself at frame-delivery-to-track time.
> > >
> > > Bug: webrtc:10668
> > > Change-Id: I03385d6865bbc7bfbef7634f88de820a934f787a
> > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139890
> > > Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> > > Reviewed-by: Minyue Li <minyue@webrtc.org>
> > > Commit-Queue: Chen Xing <chxg@google.com>
> > > Cr-Commit-Position: refs/heads/master@{#28434}
> >
> > TBR=kwiberg@webrtc.org,stefan@webrtc.org,minyue@webrtc.org,chxg@google.com
> >
> > Bug: webrtc:10668, chromium:982260
> > Change-Id: I5e2cfde78c59d1123e21869564d76ed3f6193a5c
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145339
> > Reviewed-by: Ivo Creusen <ivoc@webrtc.org>
> > Commit-Queue: Ivo Creusen <ivoc@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#28561}
>
> TBR=kwiberg@webrtc.org,stefan@webrtc.org,ivoc@webrtc.org,minyue@webrtc.org,chxg@google.com
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: webrtc:10668, chromium:982260
> Change-Id: Ie375a0b327ee368317bf3a04b2f1415c3a974470
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146707
> Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> Commit-Queue: Chen Xing <chxg@google.com>
> Cr-Commit-Position: refs/heads/master@{#28664}
TBR=kwiberg@webrtc.org,stefan@webrtc.org,ivoc@webrtc.org,minyue@webrtc.org,chxg@google.com
Change-Id: I652cb0814d83b514d3bee34e65ca3bb693099b22
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10668, chromium:982260
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146712
Reviewed-by: Alessio Bazzica <alessiob@webrtc.org>
Commit-Queue: Alessio Bazzica <alessiob@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28671}
diff --git a/modules/audio_coding/neteq/neteq_impl_unittest.cc b/modules/audio_coding/neteq/neteq_impl_unittest.cc
index ded54bf..517f4ac 100644
--- a/modules/audio_coding/neteq/neteq_impl_unittest.cc
+++ b/modules/audio_coding/neteq/neteq_impl_unittest.cc
@@ -11,8 +11,6 @@
#include "modules/audio_coding/neteq/neteq_impl.h"
#include <memory>
-#include <utility>
-#include <vector>
#include "absl/memory/memory.h"
#include "api/audio_codecs/builtin_audio_decoder_factory.h"
@@ -33,7 +31,6 @@
#include "modules/audio_coding/neteq/sync_buffer.h"
#include "modules/audio_coding/neteq/timestamp_scaler.h"
#include "rtc_base/numerics/safe_conversions.h"
-#include "system_wrappers/include/clock.h"
#include "test/audio_decoder_proxy_factory.h"
#include "test/function_audio_decoder_factory.h"
#include "test/gmock.h"
@@ -44,17 +41,14 @@
using ::testing::_;
using ::testing::AtLeast;
using ::testing::DoAll;
-using ::testing::ElementsAre;
using ::testing::InSequence;
using ::testing::Invoke;
-using ::testing::IsEmpty;
using ::testing::IsNull;
using ::testing::Pointee;
using ::testing::Return;
using ::testing::ReturnNull;
using ::testing::SetArgPointee;
using ::testing::SetArrayArgument;
-using ::testing::SizeIs;
using ::testing::WithArg;
namespace webrtc {
@@ -69,12 +63,12 @@
class NetEqImplTest : public ::testing::Test {
protected:
- NetEqImplTest() : clock_(0) { config_.sample_rate_hz = 8000; }
+ NetEqImplTest() { config_.sample_rate_hz = 8000; }
void CreateInstance(
const rtc::scoped_refptr<AudioDecoderFactory>& decoder_factory) {
ASSERT_TRUE(decoder_factory);
- NetEqImpl::Dependencies deps(config_, &clock_, decoder_factory);
+ NetEqImpl::Dependencies deps(config_, decoder_factory);
// Get a local pointer to NetEq's TickTimer object.
tick_timer_ = deps.tick_timer.get();
@@ -224,10 +218,6 @@
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- // DTMF packets are immediately consumed by |InsertPacket()| and won't be
- // returned by |GetAudio()|.
- EXPECT_THAT(output.packet_infos_, IsEmpty());
-
// Verify first 64 samples of actual output.
const std::vector<int16_t> kOutput(
{0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
@@ -243,7 +233,6 @@
std::unique_ptr<NetEqImpl> neteq_;
NetEq::Config config_;
- SimulatedClock clock_;
TickTimer* tick_timer_ = nullptr;
MockBufferLevelFilter* mock_buffer_level_filter_ = nullptr;
BufferLevelFilter* buffer_level_filter_ = nullptr;
@@ -275,9 +264,7 @@
// TODO(hlundin): Move to separate file?
TEST(NetEq, CreateAndDestroy) {
NetEq::Config config;
- SimulatedClock clock(0);
- NetEq* neteq =
- NetEq::Create(config, &clock, CreateBuiltinAudioDecoderFactory());
+ NetEq* neteq = NetEq::Create(config, CreateBuiltinAudioDecoderFactory());
delete neteq;
}
@@ -469,10 +456,6 @@
rtp_header.sequenceNumber = 0x1234;
rtp_header.timestamp = 0x12345678;
rtp_header.ssrc = 0x87654321;
- rtp_header.numCSRCs = 3;
- rtp_header.arrOfCSRCs[0] = 43;
- rtp_header.arrOfCSRCs[1] = 65;
- rtp_header.arrOfCSRCs[2] = 17;
// This is a dummy decoder that produces as many output samples as the input
// has bytes. The output is an increasing series, starting at 1 for the first
@@ -516,8 +499,6 @@
SdpAudioFormat("L16", 8000, 1)));
// Insert one packet.
- clock_.AdvanceTimeMilliseconds(123456);
- int64_t expected_receive_time_ms = clock_.TimeInMilliseconds();
EXPECT_EQ(NetEq::kOK,
neteq_->InsertPacket(rtp_header, payload, kReceiveTime));
@@ -531,17 +512,6 @@
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- // Verify |output.packet_infos_|.
- ASSERT_THAT(output.packet_infos_, SizeIs(1));
- {
- const auto& packet_info = output.packet_infos_[0];
- EXPECT_EQ(packet_info.ssrc(), rtp_header.ssrc);
- EXPECT_THAT(packet_info.csrcs(), ElementsAre(43, 65, 17));
- EXPECT_EQ(packet_info.rtp_timestamp(), rtp_header.timestamp);
- EXPECT_FALSE(packet_info.audio_level().has_value());
- EXPECT_EQ(packet_info.receive_time_ms(), expected_receive_time_ms);
- }
-
// Start with a simple check that the fake decoder is behaving as expected.
EXPECT_EQ(kPayloadLengthSamples,
static_cast<size_t>(decoder_.next_value() - 1));
@@ -589,8 +559,6 @@
rtp_header.sequenceNumber = 0x1234;
rtp_header.timestamp = 0x12345678;
rtp_header.ssrc = 0x87654321;
- rtp_header.extension.hasAudioLevel = true;
- rtp_header.extension.audioLevel = 42;
EXPECT_CALL(mock_decoder, Reset()).WillRepeatedly(Return());
EXPECT_CALL(mock_decoder, SampleRateHz())
@@ -613,8 +581,6 @@
SdpAudioFormat("L16", 8000, 1)));
// Insert one packet.
- clock_.AdvanceTimeMilliseconds(123456);
- int64_t expected_receive_time_ms = clock_.TimeInMilliseconds();
EXPECT_EQ(NetEq::kOK,
neteq_->InsertPacket(rtp_header, payload, kReceiveTime));
@@ -627,32 +593,16 @@
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- // Verify |output.packet_infos_|.
- ASSERT_THAT(output.packet_infos_, SizeIs(1));
- {
- const auto& packet_info = output.packet_infos_[0];
- EXPECT_EQ(packet_info.ssrc(), rtp_header.ssrc);
- EXPECT_THAT(packet_info.csrcs(), IsEmpty());
- EXPECT_EQ(packet_info.rtp_timestamp(), rtp_header.timestamp);
- EXPECT_EQ(packet_info.audio_level(), rtp_header.extension.audioLevel);
- EXPECT_EQ(packet_info.receive_time_ms(), expected_receive_time_ms);
- }
-
// Insert two more packets. The first one is out of order, and is already too
// old, the second one is the expected next packet.
rtp_header.sequenceNumber -= 1;
rtp_header.timestamp -= kPayloadLengthSamples;
- rtp_header.extension.audioLevel = 1;
payload[0] = 1;
- clock_.AdvanceTimeMilliseconds(1000);
EXPECT_EQ(NetEq::kOK,
neteq_->InsertPacket(rtp_header, payload, kReceiveTime));
rtp_header.sequenceNumber += 2;
rtp_header.timestamp += 2 * kPayloadLengthSamples;
- rtp_header.extension.audioLevel = 2;
payload[0] = 2;
- clock_.AdvanceTimeMilliseconds(2000);
- expected_receive_time_ms = clock_.TimeInMilliseconds();
EXPECT_EQ(NetEq::kOK,
neteq_->InsertPacket(rtp_header, payload, kReceiveTime));
@@ -675,17 +625,6 @@
// out-of-order packet should have been discarded.
EXPECT_TRUE(packet_buffer_->Empty());
- // Verify |output.packet_infos_|. Expect to only see the second packet.
- ASSERT_THAT(output.packet_infos_, SizeIs(1));
- {
- const auto& packet_info = output.packet_infos_[0];
- EXPECT_EQ(packet_info.ssrc(), rtp_header.ssrc);
- EXPECT_THAT(packet_info.csrcs(), IsEmpty());
- EXPECT_EQ(packet_info.rtp_timestamp(), rtp_header.timestamp);
- EXPECT_EQ(packet_info.audio_level(), rtp_header.extension.audioLevel);
- EXPECT_EQ(packet_info.receive_time_ms(), expected_receive_time_ms);
- }
-
EXPECT_CALL(mock_decoder, Die());
}
@@ -722,7 +661,6 @@
EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kPLC, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, IsEmpty());
// Register the payload type.
EXPECT_TRUE(neteq_->RegisterPayloadType(kPayloadType,
@@ -745,7 +683,6 @@
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_)
<< "NetEq did not decode the packets as expected.";
- EXPECT_THAT(output.packet_infos_, SizeIs(1));
}
}
@@ -783,7 +720,6 @@
EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_NE(AudioFrame::kNormalSpeech, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, IsEmpty());
}
// Insert 10 packets.
@@ -803,7 +739,6 @@
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_)
<< "NetEq did not decode the packets as expected.";
- EXPECT_THAT(output.packet_infos_, SizeIs(1));
}
auto lifetime_stats = neteq_->GetLifetimeStatistics();
@@ -1036,14 +971,12 @@
const size_t kExpectedOutputSize = 10 * (kSampleRateHz / 1000) * kChannels;
EXPECT_EQ(kExpectedOutputSize, output.samples_per_channel_ * kChannels);
EXPECT_EQ(kChannels, output.num_channels_);
- EXPECT_THAT(output.packet_infos_, IsEmpty());
// Second call to GetAudio will decode the packet that is ok. No errors are
// expected.
EXPECT_EQ(NetEq::kOK, neteq_->GetAudio(&output, &muted));
EXPECT_EQ(kExpectedOutputSize, output.samples_per_channel_ * kChannels);
EXPECT_EQ(kChannels, output.num_channels_);
- EXPECT_THAT(output.packet_infos_, SizeIs(1));
// Die isn't called through NiceMock (since it's called by the
// MockAudioDecoder constructor), so it needs to be mocked explicitly.
@@ -1145,7 +1078,6 @@
ASSERT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, SizeIs(1));
EXPECT_CALL(mock_decoder, Die());
}
@@ -1240,7 +1172,6 @@
EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, SizeIs(2)); // 5 ms packets vs 10 ms output
// Pull audio again. Decoder fails.
EXPECT_EQ(NetEq::kFail, neteq_->GetAudio(&output, &muted));
@@ -1254,14 +1185,12 @@
EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kPLC, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, IsEmpty());
// Pull audio again, should behave normal.
EXPECT_EQ(NetEq::kOK, neteq_->GetAudio(&output, &muted));
EXPECT_EQ(kMaxOutputSize, output.samples_per_channel_);
EXPECT_EQ(1u, output.num_channels_);
EXPECT_EQ(AudioFrame::kNormalSpeech, output.speech_type_);
- EXPECT_THAT(output.packet_infos_, SizeIs(2)); // 5 ms packets vs 10 ms output
EXPECT_CALL(mock_decoder, Die());
}
@@ -1689,4 +1618,4 @@
EXPECT_EQ(kAccelerate, neteq_->last_operation_for_test());
}
-} // namespace webrtc
+}// namespace webrtc