Fixes FPS instability issue for video MediaStreamTrack

This change reverts [1] since it has been
shown that it results in an invalid frame rate when a video MST feeds
an MediaStreamTrackProcessor.

I have also verified that the issue I assumed was fixed by [1] is no longer an issue. Hence, my analysis and assumptions done when landing
[1] must have been invalid and based on a lack of understanding of
all the details.

We also remove one unittest since it was specially designed to verify
one very particular detail in [1] which (again) now has been shown to
be wrong.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/4136370

(cherry picked from commit 5192d68)

Bug: 1427193
Change-Id: I91082e2c594195413d605825812ebab3f47261d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4373637
Reviewed-by: Tony Herre <toprice@chromium.org>
Commit-Queue: Henrik Andreasson <henrika@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1122454}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4381798
Auto-Submit: Guido Urdaneta <guidou@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/branch-heads/5672@{#124}
Cr-Branched-From: 5f2a724-refs/heads/main@{#1121455}

third_party/blink/renderer/modules/mediastream/media_stream_video_source_test.cc

@@ -616,114 +616,6 @@ TEST_F(MediaStreamVideoSourceTest, DropFrameAtTooHighRateAndThenStopDropping) {
   sink.DisconnectFromTrack();
 }
 
-// This test verifies that the frame-dropping algorithm works correctly and
-// does not drop frames with a "too close" reason code after a packet has been
-// dropped due to too high frame rate. The test also continues driving frames
-// after the first session of dropped frames to ensure that not all frames are
-// dropped even at a too high frame rate.
-TEST_F(MediaStreamVideoSourceTest,
-       EmitsFrameRateTooHighDespiteTooClosePreviousDrop) {
-  constexpr int kMaxFps = 5;
-  WebMediaStreamTrack track = CreateTrackAndStartSource(640, 480, kMaxFps);
-  MockMediaStreamVideoSink sink;
-  sink.ConnectToTrack(track);
-
-  // Send one initial frame and ensure that it gets delivered. This action will
-  // reset all states (EMA filter, timestamp etc.) in the VideoFrameAdapter
-  // (VTA).
-  DeliverVideoFrameAndWaitForRenderer(100, 100, &sink);
-  EXPECT_EQ(1, sink.number_of_frames());
-
-  // Drive three frames through where the timestamps for the first two are
-  // spaced too close for the max frame rate. The second frame should be dropped
-  // and cause a notification and a `FrameRateIsHigherThanRequested` reason.
-  // The third frame is then sent with a time difference less than the allowed
-  // min delta time between two frames after the second frame. But, given that
-  // the second frame was dropped, the actual time difference between two valid
-  // frames is 14 milliseconds and it should not trigger dropped frame with
-  // reason set to `TimestampTooCloseToPrevious` but instead a second reason
-  // code of `FrameRateIsHigherThanRequested`.
-  base::RunLoop run_loop;
-  base::OnceClosure quit_closure = run_loop.QuitClosure();
-
-  EXPECT_CALL(
-      *mock_source(),
-      OnFrameDropped(media::VideoCaptureFrameDropReason::
-                         kResolutionAdapterFrameRateIsHigherThanRequested))
-      .Times(3)
-      .WillOnce(Return())
-      .WillOnce(Return())
-      .WillOnce([&] { std::move(quit_closure).Run(); });
-
-  DeliverVideoFrame(100, 100, base::Milliseconds(10));
-  DeliverVideoFrame(100, 100, base::Milliseconds(20));
-  DeliverVideoFrame(
-      100, 100,
-      base::Milliseconds(20 + VideoTrackAdapter::kMinTimeBetweenFramesMs - 1));
-  run_loop.Run();
-  EXPECT_EQ(1, sink.number_of_frames());
-
-  // At this stage the EMA filter inside the VTA is at ~21 fps given the initial
-  // too high rate. But the VTA also contains a "keep, or leak mechanism" which
-  // builds up a "keep indicator" over time also when frames are dropped.
-  // Sending one more frame at the specified max rate should therefore be
-  // delivered in this state but those after shall not since the
-  // "keep mechanism" is reset after each pass and the EMA filter still says
-  // "too high frame rate".
-  DeliverVideoFrameAndWaitForRenderer(100, 100, base::Milliseconds(200), &sink);
-  EXPECT_EQ(2, sink.number_of_frames());
-
-  // Send one more frame close enough to the previous one to trigger a
-  // frame drop with reason set to `TooClose`. No states in the VTA should be
-  // updated and the EMA filter is now at ~19 fps given the frame that passed.
-  base::RunLoop run_loop2;
-  base::OnceClosure quit_closure2 = run_loop2.QuitClosure();
-
-  EXPECT_CALL(*mock_source(),
-              OnFrameDropped(media::VideoCaptureFrameDropReason::
-                                 kResolutionAdapterTimestampTooCloseToPrevious))
-      .Times(1)
-      .WillOnce([&] { std::move(quit_closure2).Run(); });
-
-  DeliverVideoFrame(
-      100, 100,
-      base::Milliseconds(200 + VideoTrackAdapter::kMinTimeBetweenFramesMs - 1));
-  run_loop2.Run();
-  EXPECT_EQ(2, sink.number_of_frames());
-
-  // Drive three more frames at max fps (5 Hz) and expect the first two to be
-  // dropped with `FrameRateIsHigherThanRequested` but the third frame to pass
-  // due to the "keep mechanism" in the VTA. The estimated frame rate after
-  // this session has reduced to  ~14.5 fps. The last timestamp of the last
-  // forwarded frame is 800 ms. In total, three frames should have been
-  // delivered after this round.
-  base::RunLoop run_loop3;
-  base::OnceClosure quit_closure3 = run_loop3.QuitClosure();
-
-  EXPECT_CALL(
-      *mock_source(),
-      OnFrameDropped(media::VideoCaptureFrameDropReason::
-                         kResolutionAdapterFrameRateIsHigherThanRequested))
-      .Times(2)
-      .WillRepeatedly(Return());
-  EXPECT_CALL(sink, OnVideoFrame).Times(1).WillOnce([&](base::TimeTicks) {
-    std::move(quit_closure3).Run();
-  });
-
-  constexpr base::TimeDelta kDeltaTimestampSteadyRateDuration =
-      base::Milliseconds(200);
-  base::TimeDelta timestamp =
-      base::Milliseconds(200) + kDeltaTimestampSteadyRateDuration;
-  for (int i = 0; i < 3; ++i) {
-    DeliverVideoFrame(100, 100, timestamp);
-    timestamp += kDeltaTimestampSteadyRateDuration;
-  }
-  run_loop3.Run();
-  EXPECT_EQ(3, sink.number_of_frames());
-
-  sink.DisconnectFromTrack();
-}
-
 TEST_F(MediaStreamVideoSourceTest, ReconfigureTrack) {
   WebMediaStreamTrack track = CreateTrackAndStartSource(
       640, 480, media::limits::kMaxFramesPerSecond - 2);

third_party/blink/renderer/modules/mediastream/video_track_adapter.cc

@@ -364,9 +364,6 @@ void VideoTrackAdapter::VideoFrameResolutionAdapter::DeliverFrame(
     return;
   }
 
-  // Update the last timestamp since the frame was approved and not dropped.
-  last_time_stamp_ = video_frame->timestamp();
-
   // If the frame is a texture not backed up by GPU memory we don't apply
   // cropping/scaling and deliver the frame as-is, leaving it up to the
   // destination to rescale it. Otherwise, cropping and scaling is soft-applied
@@ -497,6 +494,7 @@ bool VideoTrackAdapter::VideoFrameResolutionAdapter::MaybeDropFrame(
 
   // Do not drop frames if max frame rate hasn't been specified.
   if (!settings_.max_frame_rate().has_value()) {
+    last_time_stamp_ = frame.timestamp();
     return false;
   }
 
@@ -505,7 +503,8 @@ bool VideoTrackAdapter::VideoFrameResolutionAdapter::MaybeDropFrame(
   // Check if the time since the last frame is completely off.
   if (delta_ms.is_negative() || delta_ms > kMaxTimeInMsBetweenFrames) {
     DVLOG(3) << " reset timestamps";
-    // Reset fps calculation.
+    // Reset |last_time_stamp_| and fps calculation.
+    last_time_stamp_ = frame.timestamp();
     frame_rate_ = settings_.max_frame_rate().value_or(
         MediaStreamVideoSource::kDefaultFrameRate);
     DVLOG(1) << " frame rate filter initialized to " << frame_rate_ << " fps";
@@ -529,6 +528,8 @@ bool VideoTrackAdapter::VideoFrameResolutionAdapter::MaybeDropFrame(
     return true;
   }
 
+  last_time_stamp_ = frame.timestamp();
+
   // Calculate the frame rate using an exponential moving average (EMA) filter.
   // Use a simple filter with 0.1 weight of the current sample.
   frame_rate_ = 100 / delta_ms.InMillisecondsF() + 0.9 * frame_rate_;