From 28c4986e1b44b3d61f429d8bf560ce1d727bd3e4 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Fri, 3 Mar 2023 12:08:18 +0100 Subject: [PATCH] WebRTCVideoChannel::OnPacketReceived: avoid PostTasks. Under the combined network/worker thread project, tasks are unnecessarily posted to the same thread. Avoid this by posting only if invoked on a diffferent sequence. TESTED=presubmit + local Meet calls. Bug: webrtc:137439 Change-Id: Ic5dd99e5fbb843ad4c54d4466138135ae81596cf Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/295867 Commit-Queue: Markus Handell Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#39471} --- media/engine/webrtc_video_engine.cc | 67 ++++++++++++-------- media/engine/webrtc_video_engine.h | 13 ++-- media/engine/webrtc_video_engine_unittest.cc | 4 +- 3 files changed, 51 insertions(+), 33 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index ea11ae03f9..14c4979ca7 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1741,34 +1741,23 @@ void WebRtcVideoChannel::FillReceiveCodecStats( void WebRtcVideoChannel::OnPacketReceived( const webrtc::RtpPacketReceived& packet) { + // Note: the network_thread_checker may refer to the worker thread if the two + // threads are combined, but this is either always true or always false + // depending on configuration set at object initialization. RTC_DCHECK_RUN_ON(&network_thread_checker_); - // TODO(bugs.webrtc.org/11993): This code is very similar to what - // WebRtcVoiceMediaChannel::OnPacketReceived does. For maintainability and - // consistency it would be good to move the interaction with call_->Receiver() - // to a common implementation and provide a callback on the worker thread - // for the exception case (DELIVERY_UNKNOWN_SSRC) and how retry is attempted. - worker_thread_->PostTask( - SafeTask(task_safety_.flag(), [this, packet = packet]() mutable { - RTC_DCHECK_RUN_ON(&thread_checker_); - - // TODO(bugs.webrtc.org/7135): extensions in `packet` is currently set - // in RtpTransport and does not neccessarily include extensions specific - // to this channel/MID. Also see comment in - // BaseChannel::MaybeUpdateDemuxerAndRtpExtensions_w. - // It would likely be good if extensions where merged per BUNDLE and - // applied directly in RtpTransport::DemuxPacket; - packet.IdentifyExtensions(recv_rtp_extension_map_); - packet.set_payload_type_frequency(webrtc::kVideoPayloadTypeFrequency); - if (!packet.arrival_time().IsFinite()) { - packet.set_arrival_time(webrtc::Timestamp::Micros(rtc::TimeMicros())); - } - - call_->Receiver()->DeliverRtpPacket( - webrtc::MediaType::VIDEO, std::move(packet), - absl::bind_front( - &WebRtcVideoChannel::MaybeCreateDefaultReceiveStream, this)); - })); + // TODO(bugs.webrtc.org/137439): Stop posting to the worker thread when the + // combined network/worker project launches. + if (webrtc::TaskQueueBase::Current() != worker_thread_) { + worker_thread_->PostTask( + SafeTask(task_safety_.flag(), [this, packet = packet]() mutable { + RTC_DCHECK_RUN_ON(&thread_checker_); + ProcessReceivedPacket(std::move(packet)); + })); + } else { + RTC_DCHECK_RUN_ON(&thread_checker_); + ProcessReceivedPacket(packet); + } } bool WebRtcVideoChannel::MaybeCreateDefaultReceiveStream( @@ -3555,6 +3544,32 @@ WebRtcVideoChannel::FindReceiveStream(uint32_t ssrc) { return nullptr; } +// RTC_RUN_ON(worker_thread_) +void WebRtcVideoChannel::ProcessReceivedPacket( + webrtc::RtpPacketReceived packet) { + // TODO(bugs.webrtc.org/11993): This code is very similar to what + // WebRtcVoiceMediaChannel::OnPacketReceived does. For maintainability and + // consistency it would be good to move the interaction with call_->Receiver() + // to a common implementation and provide a callback on the worker thread + // for the exception case (DELIVERY_UNKNOWN_SSRC) and how retry is attempted. + // TODO(bugs.webrtc.org/7135): extensions in `packet` is currently set + // in RtpTransport and does not neccessarily include extensions specific + // to this channel/MID. Also see comment in + // BaseChannel::MaybeUpdateDemuxerAndRtpExtensions_w. + // It would likely be good if extensions where merged per BUNDLE and + // applied directly in RtpTransport::DemuxPacket; + packet.IdentifyExtensions(recv_rtp_extension_map_); + packet.set_payload_type_frequency(webrtc::kVideoPayloadTypeFrequency); + if (!packet.arrival_time().IsFinite()) { + packet.set_arrival_time(webrtc::Timestamp::Micros(rtc::TimeMicros())); + } + + call_->Receiver()->DeliverRtpPacket( + webrtc::MediaType::VIDEO, std::move(packet), + absl::bind_front(&WebRtcVideoChannel::MaybeCreateDefaultReceiveStream, + this)); +} + void WebRtcVideoChannel::SetRecordableEncodedFrameCallback( uint32_t ssrc, std::function callback) { diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index b059c39e02..495de88e1b 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -234,11 +234,6 @@ class WebRtcVideoChannel : public VideoMediaChannel, private: class WebRtcVideoReceiveStream; - // Finds VideoReceiveStreamInterface corresponding to ssrc. Aware of - // unsignalled ssrc handling. - WebRtcVideoReceiveStream* FindReceiveStream(uint32_t ssrc) - RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_); - struct VideoCodecSettings { VideoCodecSettings(); @@ -281,6 +276,14 @@ class WebRtcVideoChannel : public VideoMediaChannel, absl::optional flexfec_payload_type; }; + // Finds VideoReceiveStreamInterface corresponding to ssrc. Aware of + // unsignalled ssrc handling. + WebRtcVideoReceiveStream* FindReceiveStream(uint32_t ssrc) + RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_); + + void ProcessReceivedPacket(webrtc::RtpPacketReceived packet) + RTC_RUN_ON(thread_checker_); + bool GetChangedSendParameters(const VideoSendParameters& params, ChangedSendParameters* changed_params) const RTC_EXCLUSIVE_LOCKS_REQUIRED(thread_checker_); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 272b800bc2..c86ceb6176 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -1715,8 +1715,8 @@ TEST_F(WebRtcVideoChannelEncodedFrameCallbackTest, /*is_default_stream=*/true)); EXPECT_TRUE(channel_->SetSink(kSsrc + 1, &renderer_)); channel_->SetRecordableEncodedFrameCallback(kSsrc, callback.AsStdFunction()); - DeliverKeyFrame(kSsrc); // Expected to not cause function to fire. channel_->SetDefaultSink(&renderer_); + DeliverKeyFrame(kSsrc); // Expected to not cause function to fire. DeliverKeyFrameAndWait(kSsrc + 1); receive_channel_->RemoveRecvStream(kSsrc + 1); } @@ -2217,8 +2217,8 @@ TEST_F(WebRtcVideoChannelBaseTest, SetSink) { EXPECT_TRUE(SetDefaultCodec()); EXPECT_TRUE(SetSend(true)); EXPECT_EQ(0, renderer_.num_rendered_frames()); - channel_->OnPacketReceived(packet); channel_->SetDefaultSink(&renderer_); + channel_->OnPacketReceived(packet); SendFrame(); EXPECT_FRAME_WAIT(1, kVideoWidth, kVideoHeight, kTimeout); }