Add TransformableVideoFrameInterface::Metadata()

Add a method to TransformableVideoFrameInterface which returns a new
instance of VideoFrameMetadata which the caller can move and use as
they like.
This will replace the existing GetMetadata which returns a dangerous const ref to a field which might change if someone calls SetMetadata
etc. That method will be deprecated as soon as we've migrated Chromium
usages.

Bug: webrtc:14708
Change-Id: Id7c15f33d6ec28c4a975ce250cdc791d7a3087bc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/292940
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tony Herre <herre@google.com>
Reviewed-by: Tove Petersson <tovep@google.com>
Cr-Commit-Position: refs/heads/main@{#39403}

api/frame_transformer_interface.h

@@ -66,7 +66,11 @@ class TransformableVideoFrameInterface : public TransformableFrameInterface {
   [[deprecated("https://crbug.com/1414370")]] virtual std::vector<uint8_t>
   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&) {}

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<MockTransformableVideoFrame>, "");

modules/rtp_rtcp/source/rtp_sender_video_frame_transformer_delegate.cc

@@ -34,18 +34,18 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface {
       std::vector<uint32_t> 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();
+    ssrc_ = metadata.GetSsrc();
-    std::vector<uint32_t> csrcs = metadata.GetCsrcs();
+    csrcs_ = metadata.GetCsrcs();
-
+    // Cache a copy to allow GetMetadata() to return references.
-    // We have to keep a local copy because GetMetadata() has to return a
+    // TODO(crbug.com/webrtc/14709): Remove once GetMetadata() is removed.
-    // reference.
+    metadata_ = Metadata();
-    metadata_ = header_.GetAsMetadata();
-    metadata_.SetSsrc(ssrc);
-    metadata_.SetCsrcs(std::move(csrcs));
   }
 
   const RTPVideoHeader& GetHeader() const { return header_; }
@@ -100,11 +105,6 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface {
  private:
   rtc::scoped_refptr<EncodedImageBufferInterface> 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<VideoCodecType> codec_type_ = absl::nullopt;
@@ -112,6 +112,15 @@ class TransformableVideoSenderFrame : public TransformableVideoFrameInterface {
   const int64_t capture_time_ms_;
   const absl::optional<Timestamp> capture_time_identifier_;
   const absl::optional<int64_t> expected_retransmission_time_ms_;
+
+  uint32_t ssrc_;
+  std::vector<uint32_t> 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
 

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<RTPSenderVideoFrameTransformerDelegate>(
+      &test_sender_, frame_transformer_,
+      /*ssrc=*/1111, /*csrcs=*/std::vector<uint32_t>({1, 2, 3}),
+      time_controller_.CreateTaskQueueFactory().get());
+
+  std::unique_ptr<TransformableFrameInterface> frame =
+      GetTransformableFrame(delegate);
+  ASSERT_TRUE(frame);
+  TransformableVideoFrameInterface* video_frame =
+      static_cast<TransformableVideoFrameInterface*>(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<RTPSenderVideoFrameTransformerDelegate>(
+      &test_sender_, frame_transformer_,
+      /*ssrc=*/1111, /*csrcs=*/std::vector<uint32_t>(),
+      time_controller_.CreateTaskQueueFactory().get());
+
+  std::unique_ptr<TransformableFrameInterface> frame =
+      GetTransformableFrame(delegate);
+  ASSERT_TRUE(frame);
+  TransformableVideoFrameInterface* video_frame =
+      static_cast<TransformableVideoFrameInterface*>(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

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";