Revert "Propagate VideoFrame::UpdateRect to encoder"

This reverts commit efa72a1312e9871c9b33b7a1fec208b379608898.

Reason for revert: It seems to break come chromium.webrtc.fyi bots:

They are all release.

https://ci.chromium.org/p/chromium/builders/luci.chromium.webrtc.fyi/WebRTC%20Chromium%20FYI%20Linux%20Tester/2167
https://ci.chromium.org/p/chromium/builders/luci.chromium.webrtc.fyi/WebRTC%20Chromium%20FYI%20Mac%20Tester/1833
https://ci.chromium.org/p/chromium/builders/luci.chromium.webrtc.fyi/WebRTC%20Chromium%20FYI%20Win10%20Tester/1835

Original change's description:
> 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 occusion then
> configuration have changed.
> 
> Bug: webrtc:10310
> Change-Id: I2813ecd009eb730bd99ffa0a02f979091b56bf80
> Reviewed-on: https://webrtc-review.googlesource.com/c/123102
> Commit-Queue: Ilya Nikolaevskiy <ilnik@webrtc.org>
> Reviewed-by: Niels Moller <nisse@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#26711}

TBR=ilnik@webrtc.org,nisse@webrtc.org,sprang@webrtc.org

Change-Id: If34b5440393fffba6c37cd80c02e2b419b7ec601
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:10310
Reviewed-on: https://webrtc-review.googlesource.com/c/123224
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26719}

api/video/video_frame.cc

@@ -10,55 +10,11 @@
 
 #include "api/video/video_frame.h"
 
-#include <algorithm>
-
 #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;

api/video/video_frame.h

@@ -31,17 +31,6 @@ 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.

media/base/video_broadcaster.cc

@@ -28,10 +28,6 @@ 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();
 }
@@ -56,7 +52,6 @@ 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) {
@@ -65,8 +60,6 @@ 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) {
@@ -79,23 +72,10 @@ 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.
-      // Update_rect is not set here for this reason.
-      webrtc::VideoFrame copy =
-          webrtc::VideoFrame::Builder()
-              .set_video_frame_buffer(frame.video_frame_buffer())
-              .set_rotation(frame.rotation())
-              .set_timestamp_us(frame.timestamp_us())
-              .set_id(frame.id())
-              .set_color_space(frame.color_space())
-              .build();
-      sink_pair.sink->OnFrame(copy);
     } else {
       sink_pair.sink->OnFrame(frame);
     }
   }
-  previous_frame_sent_to_all_sinks_ = !current_frame_was_discarded;
 }
 
 void VideoBroadcaster::OnDiscardedFrame() {

media/base/video_broadcaster.h

@@ -60,8 +60,6 @@ class VideoBroadcaster : public VideoSourceBase,
 
   VideoSinkWants current_wants_ RTC_GUARDED_BY(sinks_and_wants_lock_);
   rtc::scoped_refptr<webrtc::VideoFrameBuffer> black_frame_buffer_;
-  bool previous_frame_sent_to_all_sinks_ RTC_GUARDED_BY(sinks_and_wants_lock_) =
-      true;
 };
 
 }  // namespace rtc

media/engine/simulcast_encoder_adapter.cc

@@ -418,8 +418,6 @@ 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())

modules/video_coding/video_sender.cc

@@ -199,10 +199,6 @@ 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())

video/video_stream_encoder.cc

@@ -388,7 +388,6 @@ 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),
@@ -759,10 +758,6 @@ 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;
   }
 
@@ -795,7 +790,6 @@ 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 "
@@ -884,9 +878,6 @@ 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,
@@ -914,14 +905,6 @@ 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);
@@ -938,7 +921,6 @@ 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;
   }
@@ -956,7 +938,6 @@ 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;
   }
@@ -976,7 +957,6 @@ 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;
   }
 
@@ -997,20 +977,13 @@ 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)
@@ -1018,24 +991,8 @@ 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(),

video/video_stream_encoder.h

@@ -274,9 +274,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface,
   absl::optional<VideoFrame> 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<int64_t> last_parameters_update_ms_

video/video_stream_encoder_unittest.cc

@@ -373,22 +373,6 @@ 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<TestBuffer>(
-                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()
@@ -589,11 +573,6 @@ 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,
@@ -611,7 +590,6 @@ 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);
@@ -677,8 +655,6 @@ 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 {
@@ -3385,48 +3361,4 @@ 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