From f921d2532026111d427277182d3838f86e47ceb7 Mon Sep 17 00:00:00 2001 From: Tony Herre Date: Mon, 27 Nov 2023 10:37:06 +0100 Subject: [PATCH] Remove DCHECK on setting audio rcvr encoded transform twice Failing a DCHECK on a ChannelReceiver having its encoded transform set more than once contradicts the comment above - this can happen when reconfiguring a channel, eg as in the web platform test webrtc/recvonly-transceiver-can-become-sendrecv.https.html. It was added after the original code by a different author, and indeed the video side doesn't have such a check. Bug: chromium:1502781 Change-Id: Id36e52601da34ebc194ff058e4672046379f576e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328560 Commit-Queue: Tony Herre Auto-Submit: Tony Herre Commit-Queue: Harald Alvestrand Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#41246} --- audio/BUILD.gn | 1 + audio/channel_receive.cc | 13 +++++-- ...nnel_receive_frame_transformer_delegate.cc | 7 ++++ ...annel_receive_frame_transformer_delegate.h | 2 ++ audio/channel_receive_unittest.cc | 36 +++++++++++++++++++ 5 files changed, 56 insertions(+), 3 deletions(-) diff --git a/audio/BUILD.gn b/audio/BUILD.gn index ec09e5a350..bbc294bd75 100644 --- a/audio/BUILD.gn +++ b/audio/BUILD.gn @@ -241,6 +241,7 @@ if (rtc_include_tests) { "../rtc_base:logging", "../rtc_base:threading", "../test:audio_codec_mocks", + "../test:mock_frame_transformer", "../test:mock_transport", "../test:test_support", "../test/time_controller", diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index efd9668c18..8367b00113 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -925,12 +925,19 @@ void ChannelReceive::SetAssociatedSendChannel( void ChannelReceive::SetDepacketizerToDecoderFrameTransformer( rtc::scoped_refptr frame_transformer) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); - // Depending on when the channel is created, the transformer might be set - // twice. Don't replace the delegate if it was already initialized. - if (!frame_transformer || frame_transformer_delegate_) { + if (!frame_transformer) { RTC_DCHECK_NOTREACHED() << "Not setting the transformer?"; return; } + if (frame_transformer_delegate_) { + // Depending on when the channel is created, the transformer might be set + // twice. Don't replace the delegate if it was already initialized. + // TODO(crbug.com/webrtc/15674): Prevent multiple calls during + // reconfiguration. + RTC_CHECK_EQ(frame_transformer_delegate_->FrameTransformer(), + frame_transformer); + return; + } InitFrameTransformerDelegate(std::move(frame_transformer)); } diff --git a/audio/channel_receive_frame_transformer_delegate.cc b/audio/channel_receive_frame_transformer_delegate.cc index 3cad530936..8a7dda826e 100644 --- a/audio/channel_receive_frame_transformer_delegate.cc +++ b/audio/channel_receive_frame_transformer_delegate.cc @@ -157,4 +157,11 @@ void ChannelReceiveFrameTransformerDelegate::ReceiveFrame( // originally from this receiver. receive_frame_callback_(frame->GetData(), header); } + +rtc::scoped_refptr +ChannelReceiveFrameTransformerDelegate::FrameTransformer() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + return frame_transformer_; +} + } // namespace webrtc diff --git a/audio/channel_receive_frame_transformer_delegate.h b/audio/channel_receive_frame_transformer_delegate.h index ac7cef0f76..37ff75c2e9 100644 --- a/audio/channel_receive_frame_transformer_delegate.h +++ b/audio/channel_receive_frame_transformer_delegate.h @@ -62,6 +62,8 @@ class ChannelReceiveFrameTransformerDelegate : public TransformedFrameCallback { // `channel_receive_thread_`, by calling `receive_frame_callback_`. void ReceiveFrame(std::unique_ptr frame) const; + rtc::scoped_refptr FrameTransformer(); + protected: ~ChannelReceiveFrameTransformerDelegate() override = default; diff --git a/audio/channel_receive_unittest.cc b/audio/channel_receive_unittest.cc index 4b7b7c0231..aab8a95d8b 100644 --- a/audio/channel_receive_unittest.cc +++ b/audio/channel_receive_unittest.cc @@ -29,6 +29,7 @@ #include "test/gmock.h" #include "test/gtest.h" #include "test/mock_audio_decoder_factory.h" +#include "test/mock_frame_transformer.h" #include "test/mock_transport.h" #include "test/time_controller/simulated_time_controller.h" @@ -226,6 +227,41 @@ TEST_F(ChannelReceiveTest, CaptureStartTimeBecomesValid) { EXPECT_NE(ProbeCaptureStartNtpTime(*channel), -1); } +TEST_F(ChannelReceiveTest, SettingFrameTransformer) { + auto channel = CreateTestChannelReceive(); + + rtc::scoped_refptr mock_frame_transformer = + rtc::make_ref_counted(); + + EXPECT_CALL(*mock_frame_transformer, RegisterTransformedFrameCallback); + channel->SetDepacketizerToDecoderFrameTransformer(mock_frame_transformer); + + // Must start playout, otherwise packet is discarded. + channel->StartPlayout(); + + RtpPacketReceived packet = CreateRtpPacket(); + + // Receive one RTP packet, this should be transformed. + EXPECT_CALL(*mock_frame_transformer, Transform); + channel->OnRtpPacket(packet); +} + +TEST_F(ChannelReceiveTest, SettingFrameTransformerMultipleTimes) { + auto channel = CreateTestChannelReceive(); + + rtc::scoped_refptr mock_frame_transformer = + rtc::make_ref_counted(); + + EXPECT_CALL(*mock_frame_transformer, RegisterTransformedFrameCallback); + channel->SetDepacketizerToDecoderFrameTransformer(mock_frame_transformer); + + // Set the same transformer again, shouldn't cause any additional callback + // registration calls. + EXPECT_CALL(*mock_frame_transformer, RegisterTransformedFrameCallback) + .Times(0); + channel->SetDepacketizerToDecoderFrameTransformer(mock_frame_transformer); +} + } // namespace } // namespace voe } // namespace webrtc