Commit e89d907e authored by Saman Sami's avatar Saman Sami Committed by Commit Bot

Reland "Revert "Skip MISSED frames in browser compositor""

This reverts commit 1374013c and
disables some media tests that flake due to a race that is exposed
by the revert.

Reason for revert: causes jank https://crbug.com/947587
The original reason for landing this CL does not apply anymore. It is
no longer the case that viz skips a BeginFrame if an undrawn frame
exists. See https://crrev.com/c/1521967

Original change's description:
> Skip MISSED frames in browser compositor
>
> This CL fixes a dropped frame issue when a scroll transitions from
> dragging to a momentum "fling" phase. When the user is scrolling and
> lifts their finger with velocity, the scroll continues according to a
> decelerating animation curve. When this happens, the FlingController
> receives a FlingStart instead of a ScrollUpdate and it uses that to
> create an animation curve.
>
> When this happens the browser process registers itself as an animation
> observer so that it begins to receive BeginFrame signals. However, the
> display service issues a MISSED BeginFrame for the frame in which the
> animation was registered. This will frequently miss submission in the
> current frame, so it submit next frame and won't receive a BeginFrame
> then. This leads to a frame without the browser ticking the fling
> animation. See https://crbug.com/882907#c50 for a diagram and more
> complete explanation of the issue.
>
> The browser process shouldn't require processing MISSED frames so in
> this CL we simply drop MISSED frames if we're in the browser compositor
> (commit_to_active_tree is true). Longer term, the display scheduler
> should make these decisions but this is an immediate fix for the fling
> issue.
>
> Bug: 882907
> Change-Id: Ieb6d14051dc05c5e177da1920dbd27684f941d29
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1517164
> Commit-Queue: David Bokan <bokan@chromium.org>
> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#640172}

TBR=bokan@chromium.org,sunnyps@chromium.org


Bug: 947587,882907,963141
Change-Id: Id23b586f89de67f9e77212ee265f4ad2db5a6fe7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1613363Reviewed-by: default avatarSaman Sami <samans@chromium.org>
Reviewed-by: default avatarDavid Bokan <bokan@chromium.org>
Commit-Queue: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660014}
parent 3c74c504
......@@ -933,17 +933,6 @@ bool Scheduler::ShouldDropBeginFrame(const viz::BeginFrameArgs& args) const {
return true;
}
// We shouldn't be handling missed frames in the browser process (i.e. where
// commit_to_active_tree is set) since we don't know if we should create a
// frame at this time. Doing so leads to issues like crbug.com/882907. This
// early-out is a short term fix to keep fling animations smooth.
// TODO(bokan): In the long term, the display compositor should decide
// whether to issue a missed frame; it is tracked in
// https://crbug.com/930890.
if (args.type == viz::BeginFrameArgs::MISSED &&
settings_.commit_to_active_tree)
return true;
return false;
}
......
......@@ -639,26 +639,6 @@ TEST_F(SchedulerTest, VideoNeedsBeginFrames) {
EXPECT_FALSE(scheduler_->begin_frames_expected());
}
// As a short term fix for https://crbug.com/882907, we should skip MISSED
// frames from the browser compositor.
// TODO(bokan): In the long term, the display compositor should decide
// whether to issue a missed frame; it is tracked in
// https://crbug.com/930890.
TEST_F(SchedulerTest, BrowserCompositorSkipsMissedBeginFrames) {
scheduler_settings_.commit_to_active_tree = true;
SetUpScheduler(EXTERNAL_BFS);
scheduler_->SetNeedsBeginMainFrame();
task_runner_->AdvanceMockTickClock(viz::BeginFrameArgs::DefaultInterval());
viz::BeginFrameArgs args =
fake_external_begin_frame_source_->CreateBeginFrameArgs(
BEGINFRAME_FROM_HERE, task_runner_->GetMockTickClock());
args.type = viz::BeginFrameArgs::MISSED;
fake_external_begin_frame_source_->TestOnBeginFrame(args);
EXPECT_FALSE(client_->IsInsideBeginImplFrame());
}
TEST_F(SchedulerTest, RequestCommit) {
SetUpScheduler(EXTERNAL_BFS);
......
......@@ -5578,6 +5578,11 @@ crbug.com/962726 [ Linux Win ] external/wpt/event-timing/timingconditions.html [
crbug.com/937170 [ Linux Win7 ] external/wpt/IndexedDB/interleaved-cursors-large.html [ Pass Timeout ]
crbug.com/937991 [ Win7 Release ] http/tests/devtools/cache-storage/cache-data.js [ Pass Timeout ]
# These video tests seem to take a screenshot too early.
crbug.com/963141 [ Mac Win ] media/video-object-fit.html [ Pass Failure ]
crbug.com/963141 [ Mac Win ] media/video-object-fit-change.html [ Pass Failure ]
crbug.com/963141 [ Mac Win ] media/controls/video-overlay-cast-light-rendering.html [ Pass Failure ]
# Disable tests in order to land
# https://webrtc-review.googlesource.com/c/src/+/135948
crbug.com/943975 external/wpt/webrtc/RTCDataChannel-send.html [ Pass Failure ]
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment