Cleanup video frame metadata copying
In several places VideoFrame::Builder is used to create a new VideoFrame
when intent is to change only one or two fields of a const VideoFrame&.
This approach is bad because each and every metadata field have to be
added to all the places.
Instead, this CL adds missing setters and refactors the code to use
full copy of a VideoFrame and update required fields only.
Along the way few actual bugs are fixed, e.g. when ColorSpace isn't copied
when frame rotation or buffer is cropped or converted.
Bug: webrtc:10460
Change-Id: I2895a473ca938b150eed2916c689060bdf58cb25
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/140102
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28170}
diff --git a/api/video/video_frame.cc b/api/video/video_frame.cc
index 7a71f43..8040536 100644
--- a/api/video/video_frame.cc
+++ b/api/video/video_frame.cc
@@ -196,6 +196,12 @@
return video_frame_buffer_;
}
+void VideoFrame::set_video_frame_buffer(
+ const rtc::scoped_refptr<VideoFrameBuffer>& buffer) {
+ RTC_CHECK(buffer);
+ video_frame_buffer_ = buffer;
+}
+
int64_t VideoFrame::render_time_ms() const {
return timestamp_us() / rtc::kNumMicrosecsPerMillisec;
}
diff --git a/api/video/video_frame.h b/api/video/video_frame.h
index c7dd02a..5e04c1b 100644
--- a/api/video/video_frame.h
+++ b/api/video/video_frame.h
@@ -160,6 +160,9 @@
// initialized VideoFrame.
rtc::scoped_refptr<webrtc::VideoFrameBuffer> video_frame_buffer() const;
+ void set_video_frame_buffer(
+ const rtc::scoped_refptr<VideoFrameBuffer>& buffer);
+
// TODO(nisse): Deprecated.
// Return true if the frame is stored in a texture.
bool is_texture() const {
diff --git a/media/base/adapted_video_track_source.cc b/media/base/adapted_video_track_source.cc
index 9716882..c8c3222 100644
--- a/media/base/adapted_video_track_source.cc
+++ b/media/base/adapted_video_track_source.cc
@@ -51,14 +51,10 @@
if (apply_rotation() && frame.rotation() != webrtc::kVideoRotation_0 &&
buffer->type() == webrtc::VideoFrameBuffer::Type::kI420) {
/* Apply pending rotation. */
- webrtc::VideoFrame rotated_frame =
- webrtc::VideoFrame::Builder()
- .set_video_frame_buffer(webrtc::I420Buffer::Rotate(
- *buffer->GetI420(), frame.rotation()))
- .set_rotation(webrtc::kVideoRotation_0)
- .set_timestamp_us(frame.timestamp_us())
- .set_id(frame.id())
- .build();
+ webrtc::VideoFrame rotated_frame(frame);
+ rotated_frame.set_video_frame_buffer(
+ webrtc::I420Buffer::Rotate(*buffer->GetI420(), frame.rotation()));
+ rotated_frame.set_rotation(webrtc::kVideoRotation_0);
broadcaster_.OnFrame(rotated_frame);
} else {
broadcaster_.OnFrame(frame);
diff --git a/media/engine/simulcast_encoder_adapter.cc b/media/engine/simulcast_encoder_adapter.cc
index 1147d45..b7b2d76 100644
--- a/media/engine/simulcast_encoder_adapter.cc
+++ b/media/engine/simulcast_encoder_adapter.cc
@@ -420,12 +420,11 @@
// UpdateRect is not propagated to lower simulcast layers currently.
// TODO(ilnik): Consider scaling UpdateRect together with the buffer.
- VideoFrame frame = VideoFrame::Builder()
- .set_video_frame_buffer(dst_buffer)
- .set_timestamp_rtp(input_image.timestamp())
- .set_rotation(webrtc::kVideoRotation_0)
- .set_timestamp_ms(input_image.render_time_ms())
- .build();
+ VideoFrame frame(input_image);
+ frame.set_video_frame_buffer(dst_buffer);
+ frame.set_rotation(webrtc::kVideoRotation_0);
+ frame.set_update_rect(
+ VideoFrame::UpdateRect{0, 0, frame.width(), frame.height()});
int ret =
streaminfos_[stream_idx].encoder->Encode(frame, &stream_frame_types);
if (ret != WEBRTC_VIDEO_CODEC_OK) {
diff --git a/video/video_stream_decoder_impl.cc b/video/video_stream_decoder_impl.cc
index cc19f7a..b4ae848 100644
--- a/video/video_stream_decoder_impl.cc
+++ b/video/video_stream_decoder_impl.cc
@@ -280,15 +280,9 @@
timing_.StopDecodeTimer(0, *casted_decode_time_ms, decode_stop_time_ms,
frame_timestamps->render_time_us / 1000);
- callbacks_->OnDecodedFrame(
- VideoFrame::Builder()
- .set_video_frame_buffer(decoded_image.video_frame_buffer())
- .set_rotation(decoded_image.rotation())
- .set_timestamp_us(frame_timestamps->render_time_us)
- .set_timestamp_rtp(decoded_image.timestamp())
- .set_id(decoded_image.id())
- .build(),
- casted_decode_time_ms, casted_qp);
+ VideoFrame copy = decoded_image;
+ copy.set_timestamp_us(frame_timestamps->render_time_us);
+ callbacks_->OnDecodedFrame(copy, casted_decode_time_ms, casted_qp);
});
}
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index 013ad8f..fab3d57 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -1240,14 +1240,8 @@
VideoFrame::UpdateRect{0, 0, cropped_width, cropped_height};
}
}
- out_frame = VideoFrame::Builder()
- .set_video_frame_buffer(cropped_buffer)
- .set_timestamp_rtp(video_frame.timestamp())
- .set_timestamp_ms(video_frame.render_time_ms())
- .set_rotation(video_frame.rotation())
- .set_id(video_frame.id())
- .set_update_rect(update_rect)
- .build();
+ out_frame.set_video_frame_buffer(cropped_buffer);
+ out_frame.set_update_rect(update_rect);
out_frame.set_ntp_time_ms(video_frame.ntp_time_ms());
// Since accumulated_update_rect_ is constructed before cropping,
// we can't trust it. If any changes were pending, we invalidate whole
@@ -1322,14 +1316,8 @@
VideoFrame::UpdateRect{0, 0, out_frame.width(), out_frame.height()};
}
- out_frame = VideoFrame::Builder()
- .set_video_frame_buffer(converted_buffer)
- .set_timestamp_rtp(out_frame.timestamp())
- .set_timestamp_ms(out_frame.render_time_ms())
- .set_rotation(out_frame.rotation())
- .set_id(out_frame.id())
- .set_update_rect(update_rect)
- .build();
+ out_frame.set_video_frame_buffer(converted_buffer);
+ out_frame.set_update_rect(update_rect);
}
TRACE_EVENT1("webrtc", "VCMGenericEncoder::Encode", "timestamp",
