From 71aee3a116cbc5900102d4c659fc1992bff14743 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Mon, 18 Feb 2019 13:01:26 +0100 Subject: [PATCH] Reland "Propagate VideoFrame::UpdateRect to encoder" Reland with fixes for failing chromium tests. Propagate VideoFrame::UpdateRect to encoder Accumulate it in all places where frames can be dropped before they reach the encoder. Reset UpdateRect in VideoBroadcaster if frame the previous frame is dropped. No accumulation is done here since it's supposed to be a brief occasion then configuration have changed. Original Reviewed-on: https://webrtc-review.googlesource.com/c/123102 Bug: webrtc:10310 Change-Id: I18be73f47f227d6392bf9cb220b549ced225714f Reviewed-on: https://webrtc-review.googlesource.com/c/123230 Reviewed-by: Niels Moller Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#26738} --- api/video/video_frame.cc | 44 +++++++++++++++ api/video/video_frame.h | 11 ++++ media/base/video_broadcaster.cc | 14 +++++ media/base/video_broadcaster.h | 2 + media/engine/simulcast_encoder_adapter.cc | 2 + modules/video_coding/video_sender.cc | 4 ++ video/video_stream_encoder.cc | 43 ++++++++++++++ video/video_stream_encoder.h | 3 + video/video_stream_encoder_unittest.cc | 68 +++++++++++++++++++++++ 9 files changed, 191 insertions(+) diff --git a/api/video/video_frame.cc b/api/video/video_frame.cc index e91eedede8..7a71f43493 100644 --- a/api/video/video_frame.cc +++ b/api/video/video_frame.cc @@ -10,11 +10,55 @@ #include "api/video/video_frame.h" +#include + #include "rtc_base/checks.h" #include "rtc_base/time_utils.h" namespace webrtc { +void VideoFrame::UpdateRect::Union(const UpdateRect& other) { + if (other.IsEmpty()) + return; + if (IsEmpty()) { + *this = other; + return; + } + int right = std::max(offset_x + width, other.offset_x + other.width); + int bottom = std::max(offset_y + height, other.offset_y + other.height); + offset_x = std::min(offset_x, other.offset_x); + offset_y = std::min(offset_y, other.offset_y); + width = right - offset_x; + height = bottom - offset_y; + RTC_DCHECK_GT(width, 0); + RTC_DCHECK_GT(height, 0); +} + +void VideoFrame::UpdateRect::Intersect(const UpdateRect& other) { + if (other.IsEmpty() || IsEmpty()) { + MakeEmptyUpdate(); + return; + } + + int right = std::min(offset_x + width, other.offset_x + other.width); + int bottom = std::min(offset_y + height, other.offset_y + other.height); + offset_x = std::max(offset_x, other.offset_x); + offset_y = std::max(offset_y, other.offset_y); + width = right - offset_x; + height = bottom - offset_y; + if (width <= 0 || height <= 0) { + MakeEmptyUpdate(); + } +} + +void VideoFrame::UpdateRect::MakeEmptyUpdate() { + width = height = offset_x = offset_y = 0; +} + +bool VideoFrame::UpdateRect::IsEmpty() const { + return width == 0 && height == 0; +} + VideoFrame::Builder::Builder() = default; VideoFrame::Builder::~Builder() = default; diff --git a/api/video/video_frame.h b/api/video/video_frame.h index 126c3d05de..c7dd02a7b1 100644 --- a/api/video/video_frame.h +++ b/api/video/video_frame.h @@ -31,6 +31,17 @@ class RTC_EXPORT VideoFrame { int offset_y; int width; int height; + + // Makes this UpdateRect a bounding box of this and other rect. + void Union(const UpdateRect& other); + + // Makes this UpdateRect an intersection of this and other rect. + void Intersect(const UpdateRect& other); + + // Sets everything to 0, making this UpdateRect a zero-size (empty) update. + void MakeEmptyUpdate(); + + bool IsEmpty() const; }; // Preferred way of building VideoFrame objects. diff --git a/media/base/video_broadcaster.cc b/media/base/video_broadcaster.cc index a2009f21b3..ab7e622ea7 100644 --- a/media/base/video_broadcaster.cc +++ b/media/base/video_broadcaster.cc @@ -28,6 +28,10 @@ void VideoBroadcaster::AddOrUpdateSink( const VideoSinkWants& wants) { RTC_DCHECK(sink != nullptr); rtc::CritScope cs(&sinks_and_wants_lock_); + if (!FindSinkPair(sink)) { + // |Sink| is a new sink, which didn't receive previous frame. + previous_frame_sent_to_all_sinks_ = false; + } VideoSourceBase::AddOrUpdateSink(sink, wants); UpdateWants(); } @@ -52,6 +56,7 @@ VideoSinkWants VideoBroadcaster::wants() const { void VideoBroadcaster::OnFrame(const webrtc::VideoFrame& frame) { rtc::CritScope cs(&sinks_and_wants_lock_); + bool current_frame_was_discarded = false; for (auto& sink_pair : sink_pairs()) { if (sink_pair.wants.rotation_applied && frame.rotation() != webrtc::kVideoRotation_0) { @@ -60,6 +65,8 @@ void VideoBroadcaster::OnFrame(const webrtc::VideoFrame& frame) { // with rotation still pending. Protect sinks that don't expect any // pending rotation. RTC_LOG(LS_VERBOSE) << "Discarding frame with unexpected rotation."; + sink_pair.sink->OnDiscardedFrame(); + current_frame_was_discarded = true; continue; } if (sink_pair.wants.black_frames) { @@ -72,10 +79,17 @@ void VideoBroadcaster::OnFrame(const webrtc::VideoFrame& frame) { .set_id(frame.id()) .build(); sink_pair.sink->OnFrame(black_frame); + } else if (!previous_frame_sent_to_all_sinks_) { + // Since last frame was not sent to some sinks, full update is needed. + webrtc::VideoFrame copy = frame; + copy.set_update_rect( + webrtc::VideoFrame::UpdateRect{0, 0, frame.width(), frame.height()}); + sink_pair.sink->OnFrame(copy); } else { sink_pair.sink->OnFrame(frame); } } + previous_frame_sent_to_all_sinks_ = !current_frame_was_discarded; } void VideoBroadcaster::OnDiscardedFrame() { diff --git a/media/base/video_broadcaster.h b/media/base/video_broadcaster.h index 20111bcac6..898ef2ac9a 100644 --- a/media/base/video_broadcaster.h +++ b/media/base/video_broadcaster.h @@ -60,6 +60,8 @@ class VideoBroadcaster : public VideoSourceBase, VideoSinkWants current_wants_ RTC_GUARDED_BY(sinks_and_wants_lock_); rtc::scoped_refptr black_frame_buffer_; + bool previous_frame_sent_to_all_sinks_ RTC_GUARDED_BY(sinks_and_wants_lock_) = + true; }; } // namespace rtc diff --git a/media/engine/simulcast_encoder_adapter.cc b/media/engine/simulcast_encoder_adapter.cc index 3d184bdf01..d5474405cf 100644 --- a/media/engine/simulcast_encoder_adapter.cc +++ b/media/engine/simulcast_encoder_adapter.cc @@ -418,6 +418,8 @@ int SimulcastEncoderAdapter::Encode( dst_buffer->StrideV(), dst_width, dst_height, libyuv::kFilterBilinear); + // 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()) diff --git a/modules/video_coding/video_sender.cc b/modules/video_coding/video_sender.cc index 5b3a657878..4493dd7c67 100644 --- a/modules/video_coding/video_sender.cc +++ b/modules/video_coding/video_sender.cc @@ -199,6 +199,10 @@ int32_t VideoSender::AddVideoFrame( RTC_LOG(LS_ERROR) << "Frame conversion failed, dropping frame."; return VCM_PARAMETER_ERROR; } + + // UpdatedRect is not propagated because buffer was converted, + // therefore we can't guarantee that pixels outside of UpdateRect didn't + // change comparing to the previous frame. converted_frame = VideoFrame::Builder() .set_video_frame_buffer(converted_buffer) .set_timestamp_rtp(converted_frame.timestamp()) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index fb9dd8b8f1..8436dd8a3d 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -388,6 +388,7 @@ VideoStreamEncoder::VideoStreamEncoder( captured_frame_count_(0), dropped_frame_count_(0), pending_frame_post_time_us_(0), + accumulated_update_rect_{0, 0, 0, 0}, bitrate_observer_(nullptr), force_disable_frame_dropper_(false), input_framerate_(kFrameRateAvergingWindowSizeMs, 1000), @@ -758,6 +759,10 @@ void VideoStreamEncoder::OnFrame(const VideoFrame& video_frame) { << incoming_frame.ntp_time_ms() << " <= " << last_captured_timestamp_ << ") for incoming frame. Dropping."; + encoder_queue_.PostTask([this, incoming_frame]() { + RTC_DCHECK_RUN_ON(&encoder_queue_); + accumulated_update_rect_.Union(incoming_frame.update_rect()); + }); return; } @@ -790,6 +795,7 @@ void VideoStreamEncoder::OnFrame(const VideoFrame& video_frame) { ++dropped_frame_count_; encoder_stats_observer_->OnFrameDropped( VideoStreamEncoderObserver::DropReason::kEncoderQueue); + accumulated_update_rect_.Union(incoming_frame.update_rect()); } if (log_stats) { RTC_LOG(LS_INFO) << "Number of frames: captured " @@ -878,6 +884,9 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, << last_frame_info_->width << "x" << last_frame_info_->height << ", texture=" << last_frame_info_->is_texture << "."; + // Force full frame update, since resolution has changed. + accumulated_update_rect_ = + VideoFrame::UpdateRect{0, 0, video_frame.width(), video_frame.height()}; } // We have to create then encoder before the frame drop logic, @@ -905,6 +914,14 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, last_parameters_update_ms_.emplace(now_ms); } + // Because pending frame will be dropped in any case, we need to + // remember its updated region. + if (pending_frame_) { + encoder_stats_observer_->OnFrameDropped( + VideoStreamEncoderObserver::DropReason::kEncoderQueue); + accumulated_update_rect_.Union(pending_frame_->update_rect()); + } + if (DropDueToSize(video_frame.size())) { RTC_LOG(LS_INFO) << "Dropping frame. Too large for target bitrate."; int count = GetConstAdaptCounter().ResolutionCount(kQuality); @@ -921,6 +938,7 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, } else { // Ensure that any previously stored frame is dropped. pending_frame_.reset(); + accumulated_update_rect_.Union(video_frame.update_rect()); } return; } @@ -938,6 +956,7 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, // Ensure that any previously stored frame is dropped. pending_frame_.reset(); TraceFrameDropStart(); + accumulated_update_rect_.Union(video_frame.update_rect()); } return; } @@ -957,6 +976,7 @@ void VideoStreamEncoder::MaybeEncodeVideoFrame(const VideoFrame& video_frame, << ", input frame rate " << framerate_fps; OnDroppedFrame( EncodedImageCallback::DropReason::kDroppedByMediaOptimizations); + accumulated_update_rect_.Union(video_frame.update_rect()); return; } @@ -977,13 +997,20 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, I420Buffer::Create(cropped_width, cropped_height); // TODO(ilnik): Remove scaling if cropping is too big, as it should never // happen after SinkWants signaled correctly from ReconfigureEncoder. + VideoFrame::UpdateRect update_rect = video_frame.update_rect(); if (crop_width_ < 4 && crop_height_ < 4) { cropped_buffer->CropAndScaleFrom( *video_frame.video_frame_buffer()->ToI420(), crop_width_ / 2, crop_height_ / 2, cropped_width, cropped_height); + update_rect.offset_x -= crop_width_ / 2; + update_rect.offset_y -= crop_height_ / 2; + update_rect.Intersect( + VideoFrame::UpdateRect{0, 0, cropped_width, cropped_height}); + } else { cropped_buffer->ScaleFrom( *video_frame.video_frame_buffer()->ToI420().get()); + update_rect = VideoFrame::UpdateRect{0, 0, cropped_width, cropped_height}; } out_frame = VideoFrame::Builder() .set_video_frame_buffer(cropped_buffer) @@ -991,8 +1018,24 @@ void VideoStreamEncoder::EncodeVideoFrame(const VideoFrame& video_frame, .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_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 + // frame here. + if (!accumulated_update_rect_.IsEmpty()) { + accumulated_update_rect_ = + VideoFrame::UpdateRect{0, 0, out_frame.width(), out_frame.height()}; + } + } + + if (!accumulated_update_rect_.IsEmpty()) { + accumulated_update_rect_.Union(out_frame.update_rect()); + accumulated_update_rect_.Intersect( + VideoFrame::UpdateRect{0, 0, out_frame.width(), out_frame.height()}); + out_frame.set_update_rect(accumulated_update_rect_); + accumulated_update_rect_.MakeEmptyUpdate(); } TRACE_EVENT_ASYNC_STEP0("webrtc", "Video", video_frame.render_time_ms(), diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index feed55b9bf..861028f104 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -274,6 +274,9 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, absl::optional pending_frame_ RTC_GUARDED_BY(&encoder_queue_); int64_t pending_frame_post_time_us_ RTC_GUARDED_BY(&encoder_queue_); + VideoFrame::UpdateRect accumulated_update_rect_ + RTC_GUARDED_BY(&encoder_queue_); + VideoBitrateAllocationObserver* bitrate_observer_ RTC_GUARDED_BY(&encoder_queue_); absl::optional last_parameters_update_ms_ diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 3497db842e..b419e60c2d 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -373,6 +373,22 @@ class VideoStreamEncoderTest : public ::testing::Test { return frame; } + VideoFrame CreateFrameWithUpdatedPixel(int64_t ntp_time_ms, + rtc::Event* destruction_event, + int offset_x) const { + VideoFrame frame = + VideoFrame::Builder() + .set_video_frame_buffer(new rtc::RefCountedObject( + destruction_event, codec_width_, codec_height_)) + .set_timestamp_rtp(99) + .set_timestamp_ms(99) + .set_rotation(kVideoRotation_0) + .set_update_rect({offset_x, 0, 1, 1}) + .build(); + frame.set_ntp_time_ms(ntp_time_ms); + return frame; + } + VideoFrame CreateFrame(int64_t ntp_time_ms, int width, int height) const { VideoFrame frame = VideoFrame::Builder() @@ -573,6 +589,11 @@ class VideoStreamEncoderTest : public ::testing::Test { return last_framerate_; } + VideoFrame::UpdateRect GetLastUpdateRect() { + rtc::CritScope lock(&local_crit_sect_); + return last_update_rect_; + } + private: int32_t Encode(const VideoFrame& input_image, const CodecSpecificInfo* codec_specific_info, @@ -590,6 +611,7 @@ class VideoStreamEncoderTest : public ::testing::Test { last_input_height_ = input_image.height(); block_encode = block_next_encode_; block_next_encode_ = false; + last_update_rect_ = input_image.update_rect(); } int32_t result = FakeEncoder::Encode(input_image, codec_specific_info, frame_types); @@ -655,6 +677,8 @@ class VideoStreamEncoderTest : public ::testing::Test { bool force_init_encode_failed_ RTC_GUARDED_BY(local_crit_sect_) = false; double rate_factor_ RTC_GUARDED_BY(local_crit_sect_) = 1.0; uint32_t last_framerate_ RTC_GUARDED_BY(local_crit_sect_) = 0; + VideoFrame::UpdateRect last_update_rect_ + RTC_GUARDED_BY(local_crit_sect_) = {0, 0, 0, 0}; }; class TestSink : public VideoStreamEncoder::EncoderSink { @@ -3361,4 +3385,48 @@ TEST_F(VideoStreamEncoderTest, ConfiguresCorrectFrameRate) { video_stream_encoder_->Stop(); } +TEST_F(VideoStreamEncoderTest, AccumulatesUpdateRectOnDroppedFrames) { + VideoFrame::UpdateRect rect; + video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0); + + fake_encoder_.BlockNextEncode(); + video_source_.IncomingCapturedFrame( + CreateFrameWithUpdatedPixel(1, nullptr, 0)); + WaitForEncodedFrame(1); + // On the very first frame full update should be forced. + rect = fake_encoder_.GetLastUpdateRect(); + EXPECT_EQ(rect.offset_x, 0); + EXPECT_EQ(rect.offset_y, 0); + EXPECT_EQ(rect.height, codec_height_); + EXPECT_EQ(rect.width, codec_width_); + // Here, the encoder thread will be blocked in the TestEncoder waiting for a + // call to ContinueEncode. + video_source_.IncomingCapturedFrame( + CreateFrameWithUpdatedPixel(2, nullptr, 1)); + ExpectDroppedFrame(); + video_source_.IncomingCapturedFrame( + CreateFrameWithUpdatedPixel(3, nullptr, 10)); + ExpectDroppedFrame(); + fake_encoder_.ContinueEncode(); + WaitForEncodedFrame(3); + // Updates to pixels 1 and 10 should be accumulated to one 10x1 rect. + rect = fake_encoder_.GetLastUpdateRect(); + EXPECT_EQ(rect.offset_x, 1); + EXPECT_EQ(rect.offset_y, 0); + EXPECT_EQ(rect.width, 10); + EXPECT_EQ(rect.height, 1); + + video_source_.IncomingCapturedFrame( + CreateFrameWithUpdatedPixel(4, nullptr, 0)); + WaitForEncodedFrame(4); + // Previous frame was encoded, so no accumulation should happen. + rect = fake_encoder_.GetLastUpdateRect(); + EXPECT_EQ(rect.offset_x, 0); + EXPECT_EQ(rect.offset_y, 0); + EXPECT_EQ(rect.width, 1); + EXPECT_EQ(rect.height, 1); + + video_stream_encoder_->Stop(); +} + } // namespace webrtc