Handle encoded video frames on the network thread VideoSendStreamImpl handed encoded frames to RtpVideoSender on the encoder's thread, which for hardware encoders can be one of several threads, and posted a separate task to the network thread for the padding bookkeeping. Video packetization thus ran on encoder threads, while RTCP, pacing and RTP module configuration run on the network thread, which is why RtpVideoSender and RTPSender use locks. Now the bookkeeping task also carries the frame, and RtpVideoSender::OnEncodedImage() runs on the network thread, like audio packetization since https://webrtc-review.googlesource.com/c/src/+/432600. The network thread still gets two tasks per frame, but the pacer's enqueue task is now posted from the frame task instead of from the encoder thread. Moving packetization onto the network thread takes RTP header/packet construction and frame encryption off encoder threads, avoids cross-thread contention with RTCP/pacing/feedback and the extra cross-thread wakeup for pacer enqueueing, and move-enables CodecSpecificInfo and GenericFrameInfo. Behavior changes: - The encoder always gets Result::OK, without a frame_id. Send errors, e.g. for a stopped stream, aren't reported back anymore. In-tree, only VideoStreamEncoder (pending frame drops) and the ObjC encoder wrapper read the result, and nothing reads frame_id. - A FrameEncryptor runs on the network thread, like the audio FrameEncryptor and SRTP. - The encoded buffer is held until the task runs, as with a frame transformer, and the frame's metadata is copied once. - Layer allocation updates are still applied right away on the encoder queue, so a frame that is already posted can carry an allocation that was set after the frame was encoded. Layer allocation updates and transformed frames move to the network thread in follow-ups. Part of running the RTP send path on a single sequence, so that RTPSender and RtpVideoSender no longer need locks. Bug: webrtc:42223727 Change-Id: I8538681ec8a42ac3ff92fd85c4a18281918b2555 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/507960 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Erik Språng <sprang@webrtc.org> Cr-Commit-Position: refs/heads/main@{#49210}
diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index fd0e861..c9e4f1f 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc
@@ -624,6 +624,7 @@ EncodedImageCallback::Result RtpVideoSender::OnEncodedImage( const EncodedImage& encoded_image, const CodecSpecificInfo* codec_specific_info) { + RTC_DCHECK_RUN_ON(&transport_checker_); fec_controller_->UpdateWithEncodedData(encoded_image.size(), encoded_image.frame_type()); MutexLock lock(&mutex_);
diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index bc61956..d62141e 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h
@@ -124,8 +124,7 @@ // Implements FecControllerOverride. void SetFecAllowed(bool fec_allowed) RTC_LOCKS_EXCLUDED(mutex_) override; - // Implements EncodedImageCallback. - // Returns 0 if the packet was routed / sent, -1 otherwise. + // Implements EncodedImageCallback. Called on the transport queue. EncodedImageCallback::Result OnEncodedImage( const EncodedImage& encoded_image, const CodecSpecificInfo* codec_specific_info)
diff --git a/call/rtp_video_sender_interface.h b/call/rtp_video_sender_interface.h index b15954b..ed95bdc 100644 --- a/call/rtp_video_sender_interface.h +++ b/call/rtp_video_sender_interface.h
@@ -29,6 +29,10 @@ class VideoBitrateAllocation; struct FecProtectionParams; +// Methods are called on the transport queue, which is the network thread and +// also runs the pacer, with these exceptions: +// - OnVideoLayersAllocationUpdated() is called on the encoder queue. +// - SetFecAllowed() may be called on any thread. class RtpVideoSenderInterface : public EncodedImageCallback, public FecControllerOverride { public:
diff --git a/common_video/generic_frame_descriptor/generic_frame_info.cc b/common_video/generic_frame_descriptor/generic_frame_info.cc index 168304e..6dc3e55c 100644 --- a/common_video/generic_frame_descriptor/generic_frame_info.cc +++ b/common_video/generic_frame_descriptor/generic_frame_info.cc
@@ -16,6 +16,10 @@ GenericFrameInfo::GenericFrameInfo() = default; GenericFrameInfo::GenericFrameInfo(const GenericFrameInfo&) = default; +GenericFrameInfo::GenericFrameInfo(GenericFrameInfo&&) = default; +GenericFrameInfo& GenericFrameInfo::operator=(const GenericFrameInfo&) = + default; +GenericFrameInfo& GenericFrameInfo::operator=(GenericFrameInfo&&) = default; GenericFrameInfo::~GenericFrameInfo() = default; GenericFrameInfo::Builder::Builder() = default;
diff --git a/common_video/generic_frame_descriptor/generic_frame_info.h b/common_video/generic_frame_descriptor/generic_frame_info.h index 45037b7..f14a0f6 100644 --- a/common_video/generic_frame_descriptor/generic_frame_info.h +++ b/common_video/generic_frame_descriptor/generic_frame_info.h
@@ -38,6 +38,9 @@ GenericFrameInfo(); GenericFrameInfo(const GenericFrameInfo&); + GenericFrameInfo(GenericFrameInfo&&); + GenericFrameInfo& operator=(const GenericFrameInfo&); + GenericFrameInfo& operator=(GenericFrameInfo&&); ~GenericFrameInfo(); absl::InlinedVector<CodecBufferUsage, kMaxEncoderBuffers> encoder_buffers;
diff --git a/modules/video_coding/include/video_codec_interface.cc b/modules/video_coding/include/video_codec_interface.cc index aa94dcf..cdf3c20 100644 --- a/modules/video_coding/include/video_codec_interface.cc +++ b/modules/video_coding/include/video_codec_interface.cc
@@ -19,6 +19,10 @@ } CodecSpecificInfo::CodecSpecificInfo(const CodecSpecificInfo&) = default; +CodecSpecificInfo::CodecSpecificInfo(CodecSpecificInfo&&) = default; +CodecSpecificInfo& CodecSpecificInfo::operator=(const CodecSpecificInfo&) = + default; +CodecSpecificInfo& CodecSpecificInfo::operator=(CodecSpecificInfo&&) = default; CodecSpecificInfo::~CodecSpecificInfo() = default; } // namespace webrtc
diff --git a/modules/video_coding/include/video_codec_interface.h b/modules/video_coding/include/video_codec_interface.h index cd443f7..71d27a4 100644 --- a/modules/video_coding/include/video_codec_interface.h +++ b/modules/video_coding/include/video_codec_interface.h
@@ -113,6 +113,9 @@ struct RTC_EXPORT CodecSpecificInfo { CodecSpecificInfo(); CodecSpecificInfo(const CodecSpecificInfo&); + CodecSpecificInfo(CodecSpecificInfo&&); + CodecSpecificInfo& operator=(const CodecSpecificInfo&); + CodecSpecificInfo& operator=(CodecSpecificInfo&&); ~CodecSpecificInfo(); VideoCodecType codecType;
diff --git a/video/BUILD.gn b/video/BUILD.gn index 1bc4083..263e432 100644 --- a/video/BUILD.gn +++ b/video/BUILD.gn
@@ -1903,6 +1903,7 @@ "../api/units:timestamp", "../api/video:encoded_image", "../api/video:video_bitrate_allocation", + "../api/video:video_frame", "../api/video:video_frame_type", "../api/video:video_layers_allocation", "../api/video_codecs:video_codecs_api",
diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 7a433fa..562da58 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc
@@ -861,18 +861,29 @@ activity_ = true; RTC_DCHECK(!network_queue_->IsCurrent()); - auto task_to_run_on_network = [this]() { - RTC_DCHECK_RUN_ON(&thread_checker_); - if (disable_padding_) { - disable_padding_ = false; - // To ensure that padding bitrate is propagated to the bitrate allocator. - SignalEncoderActive(); - } - }; + // Packetize and send the frame on the network thread, where the RTP modules + // and the pacer are used, instead of on the encoder thread. The copy of + // `encoded_image` shares the encoded buffer. + auto task_to_run_on_network = + [this, encoded_image, + codec_specific = codec_specific_info + ? std::make_optional(*codec_specific_info) + : std::nullopt]() { + RTC_DCHECK_RUN_ON(&thread_checker_); + if (disable_padding_) { + disable_padding_ = false; + // To ensure that padding bitrate is propagated to the bitrate + // allocator. + SignalEncoderActive(); + } + rtp_video_sender_->OnEncodedImage( + encoded_image, codec_specific ? &*codec_specific : nullptr); + }; network_queue_->PostTask(SafeTask(network_queue_safety_.flag(), std::move(task_to_run_on_network))); - return rtp_video_sender_->OnEncodedImage(encoded_image, codec_specific_info); + // The frame is sent asynchronously, so there is no send result to report. + return Result(Result::OK); } void VideoSendStreamImpl::OnFrameDropped(uint32_t rtp_timestamp,
diff --git a/video/video_send_stream_impl.h b/video/video_send_stream_impl.h index 088dd98..d3f9975 100644 --- a/video/video_send_stream_impl.h +++ b/video/video_send_stream_impl.h
@@ -173,9 +173,9 @@ void OnVideoLayersAllocationUpdated( VideoLayersAllocation allocation) override; - // Implements EncodedImageCallback. The implementation routes encoded frames - // to the `payload_router_` and `config.pre_encode_callback` if set. - // Called on an arbitrary encoder callback thread. + // Implements EncodedImageCallback. Called on an arbitrary encoder callback + // thread. The frame is posted to the network thread and passed to + // `rtp_video_sender_` there, so the result is always OK. EncodedImageCallback::Result OnEncodedImage( const EncodedImage& encoded_image, const CodecSpecificInfo* codec_specific_info) override;
diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 0412396..6395109 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc
@@ -36,6 +36,7 @@ #include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "api/video/encoded_image.h" +#include "api/video/video_codec_type.h" #include "api/video/video_frame_type.h" #include "api/video/video_layers_allocation.h" #include "api/video_codecs/spatial_layer.h" @@ -81,8 +82,10 @@ using ::testing::ElementsAreArray; using ::testing::Eq; using ::testing::Field; +using ::testing::IsNull; using ::testing::Mock; using ::testing::NiceMock; +using ::testing::Pointee; using ::testing::Return; using ::testing::SaveArg; using ::testing::Sequence; @@ -577,6 +580,54 @@ vss_impl->Stop(); } +TEST_F(VideoSendStreamImplTest, SendsEncodedImageOnNetworkThread) { + auto vss_impl = CreateVideoSendStreamImpl(TestVideoEncoderConfig()); + TaskQueueBase* const network_queue = time_controller_.GetMainThread(); + + bool sent = false; + EXPECT_CALL(rtp_video_sender_, OnEncodedImage) + .WillOnce([&](const EncodedImage& encoded_image, + const CodecSpecificInfo* codec_specific_info) { + EXPECT_TRUE(network_queue->IsCurrent()); + EXPECT_EQ(encoded_image.RtpTimestamp(), 123u); + EXPECT_THAT(codec_specific_info, + Pointee(Field(&CodecSpecificInfo::codecType, + VideoCodecType::kVideoCodecVP8))); + sent = true; + return EncodedImageCallback::Result( + EncodedImageCallback::Result::ERROR_SEND_FAILED); + }); + + encoder_queue_->PostTask([&] { + // The image and the codec specific info only live during the call. + EncodedImage encoded_image; + encoded_image.SetRtpTimestamp(123); + CodecSpecificInfo codec_specific; + codec_specific.codecType = VideoCodecType::kVideoCodecVP8; + // The frame is sent later, so the encoder gets OK even though sending + // fails. + EXPECT_EQ(static_cast<EncodedImageCallback*>(vss_impl.get()) + ->OnEncodedImage(encoded_image, &codec_specific) + .error, + EncodedImageCallback::Result::OK); + EXPECT_FALSE(sent); + }); + time_controller_.AdvanceTime(TimeDelta::Zero()); + EXPECT_TRUE(sent); +} + +TEST_F(VideoSendStreamImplTest, SendsEncodedImageWithoutCodecSpecificInfo) { + auto vss_impl = CreateVideoSendStreamImpl(TestVideoEncoderConfig()); + EXPECT_CALL(rtp_video_sender_, OnEncodedImage(_, IsNull())) + .WillOnce(Return( + EncodedImageCallback::Result(EncodedImageCallback::Result::OK))); + encoder_queue_->PostTask([&] { + static_cast<EncodedImageCallback*>(vss_impl.get()) + ->OnEncodedImage(EncodedImage(), /*codec_specific_info=*/nullptr); + }); + time_controller_.AdvanceTime(TimeDelta::Zero()); +} + TEST_F(VideoSendStreamImplTest, UpdatesObserverOnConfigurationChange) { const bool kSuspend = false; config_.suspend_below_min_bitrate = kSuspend;