diff --git a/api/frame_transformer_interface.h b/api/frame_transformer_interface.h index 16a869e83b..f841072871 100644 --- a/api/frame_transformer_interface.h +++ b/api/frame_transformer_interface.h @@ -66,7 +66,11 @@ class TransformableVideoFrameInterface : public TransformableFrameInterface { [[deprecated("https://crbug.com/1414370")]] virtual std::vector GetAdditionalData() const = 0; + // The returned const ref may become invalid due to later SetMetadata calls, + // or other modifications. Use Metadata() instead. virtual const VideoFrameMetadata& GetMetadata() const = 0; + virtual VideoFrameMetadata Metadata() const = 0; + // TODO(https://crbug.com/webrtc/14709): Make pure virtual when Chromium MOCK // has implemented this. virtual void SetMetadata(const VideoFrameMetadata&) {} diff --git a/api/test/mock_transformable_video_frame.h b/api/test/mock_transformable_video_frame.h index a848032dd3..35ce8e562a 100644 --- a/api/test/mock_transformable_video_frame.h +++ b/api/test/mock_transformable_video_frame.h @@ -40,6 +40,7 @@ class MockTransformableVideoFrame GetDirection, (), (const, override)); + MOCK_METHOD(VideoFrameMetadata, Metadata, (), (const, override)); }; static_assert(!std::is_abstract_v, ""); diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc index d36ecc04ef..4057d32e15 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc @@ -34,18 +34,18 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { std::vector csrcs) : encoded_data_(encoded_image.GetEncodedData()), header_(video_header), - metadata_(header_.GetAsMetadata()), frame_type_(encoded_image._frameType), payload_type_(payload_type), codec_type_(codec_type), timestamp_(rtp_timestamp), capture_time_ms_(encoded_image.capture_time_ms_), capture_time_identifier_(encoded_image.CaptureTimeIdentifier()), - expected_retransmission_time_ms_(expected_retransmission_time_ms) { + expected_retransmission_time_ms_(expected_retransmission_time_ms), + ssrc_(ssrc), + csrcs_(csrcs), + metadata_(Metadata()) { RTC_DCHECK_GE(payload_type_, 0); RTC_DCHECK_LE(payload_type_, 127); - metadata_.SetSsrc(ssrc); - metadata_.SetCsrcs(std::move(csrcs)); } ~TransformableVideoSenderFrame() override = default; @@ -60,7 +60,7 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { } uint32_t GetTimestamp() const override { return timestamp_; } - uint32_t GetSsrc() const override { return metadata_.GetSsrc(); } + uint32_t GetSsrc() const override { return ssrc_; } bool IsKeyFrame() const override { return frame_type_ == VideoFrameType::kVideoFrameKey; @@ -71,16 +71,21 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { } const VideoFrameMetadata& GetMetadata() const override { return metadata_; } + + VideoFrameMetadata Metadata() const override { + VideoFrameMetadata metadata = header_.GetAsMetadata(); + metadata.SetSsrc(ssrc_); + metadata.SetCsrcs(csrcs_); + return metadata; + } + void SetMetadata(const VideoFrameMetadata& metadata) override { header_.SetFromMetadata(metadata); - uint32_t ssrc = metadata.GetSsrc(); - std::vector csrcs = metadata.GetCsrcs(); - - // We have to keep a local copy because GetMetadata() has to return a - // reference. - metadata_ = header_.GetAsMetadata(); - metadata_.SetSsrc(ssrc); - metadata_.SetCsrcs(std::move(csrcs)); + ssrc_ = metadata.GetSsrc(); + csrcs_ = metadata.GetCsrcs(); + // Cache a copy to allow GetMetadata() to return references. + // TODO(crbug.com/webrtc/14709): Remove once GetMetadata() is removed. + metadata_ = Metadata(); } const RTPVideoHeader& GetHeader() const { return header_; } @@ -100,11 +105,6 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { private: rtc::scoped_refptr encoded_data_; RTPVideoHeader header_; - // This is a copy of `header_.GetAsMetadata()`, only needed because the - // interface says GetMetadata() must return a const ref rather than a value. - // TODO(crbug.com/webrtc/14709): Change the interface and delete this variable - // to reduce risk of it getting out-of-sync with `header_.GetAsMetadata()`. - VideoFrameMetadata metadata_; const VideoFrameType frame_type_; const uint8_t payload_type_; const absl::optional codec_type_ = absl::nullopt; @@ -112,6 +112,15 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface { const int64_t capture_time_ms_; const absl::optional capture_time_identifier_; const absl::optional expected_retransmission_time_ms_; + + uint32_t ssrc_; + std::vector csrcs_; + + // This is a copy of the value returned by `Metadata()`, only needed because + // the interface says GetMetadata() must return a const ref rather than a + // value. + // TODO(crbug.com/webrtc/14709): Delete once GetMetdata() is removed. + VideoFrameMetadata metadata_; }; } // namespace diff --git a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate_unittest.cc index 16c88704ad..1f722bf24b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate_unittest.cc @@ -167,5 +167,58 @@ TEST_F(RtpSenderVideoFrameTransformerDelegateTest, CloneSenderVideoFrame) { // an equality operator defined. } +TEST_F(RtpSenderVideoFrameTransformerDelegateTest, MetadataEqualsGetMetadata) { + auto delegate = rtc::make_ref_counted( + &test_sender_, frame_transformer_, + /*ssrc=*/1111, /*csrcs=*/std::vector({1, 2, 3}), + time_controller_.CreateTaskQueueFactory().get()); + + std::unique_ptr frame = + GetTransformableFrame(delegate); + ASSERT_TRUE(frame); + TransformableVideoFrameInterface* video_frame = + static_cast(frame.get()); + + const VideoFrameMetadata& metadata_ref = video_frame->GetMetadata(); + VideoFrameMetadata metadata_copy = video_frame->Metadata(); + + // TODO(bugs.webrtc.org/14708): Just EXPECT_EQ the whole Metadata once the + // equality operator lands. + EXPECT_EQ(metadata_ref.GetFrameType(), metadata_copy.GetFrameType()); + EXPECT_EQ(metadata_ref.GetFrameId(), metadata_copy.GetFrameId()); + EXPECT_EQ(metadata_ref.GetCodec(), metadata_copy.GetCodec()); + EXPECT_EQ(metadata_ref.GetSsrc(), metadata_copy.GetSsrc()); + EXPECT_EQ(metadata_ref.GetCsrcs(), metadata_copy.GetCsrcs()); +} + +TEST_F(RtpSenderVideoFrameTransformerDelegateTest, MetadataAfterSetMetadata) { + auto delegate = rtc::make_ref_counted( + &test_sender_, frame_transformer_, + /*ssrc=*/1111, /*csrcs=*/std::vector(), + time_controller_.CreateTaskQueueFactory().get()); + + std::unique_ptr frame = + GetTransformableFrame(delegate); + ASSERT_TRUE(frame); + TransformableVideoFrameInterface* video_frame = + static_cast(frame.get()); + + VideoFrameMetadata metadata; + metadata.SetFrameType(VideoFrameType::kVideoFrameKey); + metadata.SetFrameId(654); + metadata.SetSsrc(2222); + metadata.SetCsrcs({1, 2, 3}); + + video_frame->SetMetadata(metadata); + VideoFrameMetadata actual_metadata = video_frame->Metadata(); + + // TODO(bugs.webrtc.org/14708): Just EXPECT_EQ the whole Metadata once the + // equality operator lands. + EXPECT_EQ(metadata.GetFrameType(), actual_metadata.GetFrameType()); + EXPECT_EQ(metadata.GetFrameId(), actual_metadata.GetFrameId()); + EXPECT_EQ(metadata.GetSsrc(), actual_metadata.GetSsrc()); + EXPECT_EQ(metadata.GetCsrcs(), actual_metadata.GetCsrcs()); +} + } // namespace } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc index 1554625baa..e2e9fec5f1 100644 --- a/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc +++ b/modules/rtp_rtcp/source/rtp_video_stream_receiver_frame_transformer_delegate.cc @@ -56,6 +56,9 @@ class TransformableVideoReceiverFrame } const VideoFrameMetadata& GetMetadata() const override { return metadata_; } + + VideoFrameMetadata Metadata() const override { return metadata_; } + void SetMetadata(const VideoFrameMetadata&) override { RTC_DCHECK_NOTREACHED() << "TransformableVideoReceiverFrame::SetMetadata is not implemented";