Skip to content

Commit

Permalink
[100] Revert "Drop duplicate and out of order VideoFrames in MSTP"
Browse files Browse the repository at this point in the history 
This reverts commit 6ec854a.

Reason for revert: Causes frames to be dropped for a long time after the
source of a track gets switched to a new one with timestamps which reset
to 0 - eg due to changing camera resolution via a applyConstraints call
on windows, or using the "Share this tab instead" button.

Note: Partial revert to leave the histogram config and WPT in place.

Original change's description:
> Drop duplicate and out of order VideoFrames in MSTP
>
> Drop videoframes which arrive on a track with duplicate or out of order timestamps, to make
> it easier for apps to handle frames without worrying about order.
>
> Bug: 1271175
> Change-Id: I20a7312a31d8122c77dccde6d5064b27f0637db3
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3304220
> Commit-Queue: Tony Herre <toprice@chromium.org>
> Reviewed-by: Ben Wagner aka dogben <benjaminwagner@google.com>
> Reviewed-by: Johannes Kron <kron@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#946371}

(cherry picked from commit 0da3ad8)

Bug: 1271175, 1301897, 1287823
Change-Id: I127a0366dca83bc4ca5f31e0c218f919919af5f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3497696
Reviewed-by: Ben Wagner <benjaminwagner@google.com>
Commit-Queue: Tony Herre <toprice@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#977730}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3507994
Auto-Submit: Tony Herre <toprice@chromium.org>
Commit-Queue: Ben Wagner <benjaminwagner@google.com>
Cr-Commit-Position: refs/branch-heads/4896@{#377}
Cr-Branched-From: 1f63ff4-refs/heads/main@{#972766}
  • Loading branch information
Tony Herre authored and Chromium LUCI CQ committed Mar 8, 2022
1 parent a30a64a commit 6b6574a
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 97 deletions.
27 changes: 0 additions & 27 deletions ...d_party/blink/renderer/modules/breakout_box/media_stream_video_track_underlying_source.cc
View file
Expand Up @@ -5,8 +5,6 @@
#include "third_party/blink/renderer/modules/breakout_box/media_stream_video_track_underlying_source.h"

#include "base/feature_list.h"
#include "base/logging.h"
#include "base/metrics/histogram_macros.h"
#include "media/capture/video/video_capture_buffer_pool_util.h"
#include "third_party/blink/public/platform/platform.h"
#include "third_party/blink/renderer/core/execution_context/execution_context.h"
Expand Down Expand Up @@ -89,31 +87,6 @@ void MediaStreamVideoTrackUnderlyingSource::OnFrameFromTrack(
std::vector<scoped_refptr<media::VideoFrame>> /*scaled_media_frames*/,
base::TimeTicks estimated_capture_time) {
DCHECK(GetIOTaskRunner()->RunsTasksInCurrentSequence());

// The track's source may provide duplicate frames, eg if other sinks
// request a refresh frame. Drop them here as we've already provided them.
// Out of order frames aren't expected, but are also dropped defensively,
// see crbug.com(1271175).
if (media_frame->timestamp() != media::kNoTimestamp &&
media_frame->timestamp() <= last_enqueued_timestamp) {
if (media_frame->timestamp() < last_enqueued_timestamp) {
DLOG(WARNING)
<< "Received unexpectedly out-of-order frame with timestamp: "
<< media_frame->timestamp()
<< ", previous timestamp: " << last_enqueued_timestamp
<< ". Dropping it.";
if (!reported_out_of_order_timestamp) {
// Monitor the number of instances performing these drops. Can be
// compared with the "Media.BreakoutBox.Usage" histogram to give a
// baseline.
UMA_HISTOGRAM_BOOLEAN("Media.BreakoutBox.OutOfOrderFrameDropped", true);
reported_out_of_order_timestamp = true;
}
}
return;
}
last_enqueued_timestamp = media_frame->timestamp();

// The scaled video frames are currently ignored.
QueueFrame(std::move(media_frame));
}
Expand Down
5 changes: 0 additions & 5 deletions third_party/blink/renderer/modules/breakout_box/media_stream_video_track_underlying_source.h
View file
Expand Up @@ -7,7 +7,6 @@

#include "base/gtest_prod_util.h"
#include "base/sequence_checker.h"
#include "media/base/timestamp_constants.h"
#include "third_party/blink/public/web/modules/mediastream/media_stream_video_sink.h"
#include "third_party/blink/renderer/core/streams/readable_stream_transferring_optimizer.h"
#include "third_party/blink/renderer/modules/breakout_box/frame_queue_underlying_source.h"
Expand Down Expand Up @@ -74,10 +73,6 @@ class MODULES_EXPORT MediaStreamVideoTrackUnderlyingSource

const Member<MediaStreamComponent> track_;

// State for handling duplicate frames. Only accessed from the IO thread.
base::TimeDelta last_enqueued_timestamp = media::kNoTimestamp;
bool reported_out_of_order_timestamp = false;

SEQUENCE_CHECKER(sequence_checker_);
};

Expand Down
69 changes: 4 additions & 65 deletions ...ty/blink/renderer/modules/breakout_box/media_stream_video_track_underlying_source_test.cc
View file
Expand Up @@ -396,9 +396,8 @@ TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, FrameLimiter) {
};

Vector<scoped_refptr<media::VideoFrame>> frames;
auto create_video_frame = [&](const base::TimeDelta timestamp) {
auto create_video_frame = [&]() {
auto frame = media::VideoFrame::CreateBlackFrame(gfx::Size(10, 10));
frame->set_timestamp(timestamp);
frames.push_back(frame);
return frame;
};
Expand All @@ -407,15 +406,15 @@ TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, FrameLimiter) {
EXPECT_TRUE(monitor.IsEmpty());
// These frames are queued, pending to be read.
for (size_t i = 0; i < max_frame_count; ++i) {
auto video_frame = create_video_frame(base::Seconds(i));
auto video_frame = create_video_frame();
int frame_id = video_frame->unique_id();
push_frame_sync(std::move(video_frame));
EXPECT_EQ(monitor.NumFrames(device_id), i + 1);
EXPECT_EQ(monitor.NumRefs(device_id, frame_id), 1);
}
{
// Push another video frame with the limit reached.
auto video_frame = create_video_frame(base::Seconds(max_frame_count));
auto video_frame = create_video_frame();
int frame_id = video_frame->unique_id();
push_frame_sync(std::move(video_frame));
EXPECT_EQ(monitor.NumFrames(device_id), max_frame_count);
Expand All @@ -442,7 +441,7 @@ TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, FrameLimiter) {
// A new frame arrives, but the limit has been reached and there is nothing
// that can be replaced.
{
auto video_frame = create_video_frame(base::Seconds(max_frame_count + 1));
auto video_frame = create_video_frame();
int frame_id = video_frame->unique_id();
push_frame_sync(std::move(video_frame));
EXPECT_EQ(monitor.NumFrames(device_id), max_frame_count);
Expand Down Expand Up @@ -535,64 +534,4 @@ TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, FrameLimiter) {
EXPECT_TRUE(monitor.IsEmpty());
}

TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, DropDuplicateFrames) {
V8TestingScope v8_scope;
ScriptState* script_state = v8_scope.GetScriptState();
auto* track = CreateTrack(v8_scope.GetExecutionContext());
auto* source = CreateSource(script_state, track, /*buffer_size=*/2);
auto* stream =
ReadableStream::CreateWithCountQueueingStrategy(script_state, source, 0);

// Push a frame at timestamp 1 multiple times, followed by one at timestamp 2.
base::TimeDelta timestamp1 = base::Seconds(1);
PushFrame(timestamp1);
PushFrame(timestamp1);
PushFrame(timestamp1);
PushFrame(base::Seconds(2));

NonThrowableExceptionState exception_state;
auto* reader =
stream->GetDefaultReaderForTesting(script_state, exception_state);

// The duplicate timestamp frames should have been discarded, leaving only
// those with distinct times.
for (wtf_size_t i = 1; i <= 2; ++i) {
VideoFrame* video_frame =
ReadObjectFromStream<VideoFrame>(v8_scope, reader);
EXPECT_EQ(base::Microseconds(*video_frame->timestamp()), base::Seconds(i));
}

source->Close();
track->stopTrack(v8_scope.GetExecutionContext());
}

TEST_F(MediaStreamVideoTrackUnderlyingSourceTest, DropOutOfOrderFrames) {
V8TestingScope v8_scope;
ScriptState* script_state = v8_scope.GetScriptState();
auto* track = CreateTrack(v8_scope.GetExecutionContext());
auto* source = CreateSource(script_state, track, /*buffer_size=*/2);
auto* stream =
ReadableStream::CreateWithCountQueueingStrategy(script_state, source, 0);

// Push an out of order frame between timestamps 2 and 3.
PushFrame(base::Seconds(2));
PushFrame(base::Seconds(1));
PushFrame(base::Seconds(3));

NonThrowableExceptionState exception_state;
auto* reader =
stream->GetDefaultReaderForTesting(script_state, exception_state);

// The out of order frame should have been discarded, leaving timestamps 2
// and 3.
for (wtf_size_t i = 2; i <= 3; ++i) {
VideoFrame* video_frame =
ReadObjectFromStream<VideoFrame>(v8_scope, reader);
EXPECT_EQ(base::Microseconds(*video_frame->timestamp()), base::Seconds(i));
}

source->Close();
track->stopTrack(v8_scope.GetExecutionContext());
}

} // namespace blink

0 comments on commit 6b6574a

Please sign in to comment.