New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 899300 link

Starred by 1 user
Status: Fixed
Owner:
kylec...@chromium.org
Closed: Nov 2
Cc:
danakj@chromium.org
█ fsam...@chromium.org
sadrul@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Compositor::refresh_rate() is wrong with OOP-D enabled

Project Member Reported by kylec...@chromium.org, Oct 26 
The |refresh_rate_| variable in ui::Compositor is only updated Compositor::SetDisplayVSyncParameters() is called. This will never happen with OOP-D enabled on Chrome OS where everything vsync/begin frame related is moved to the GPU process.

Compositor::refresh_rate() is only used on Chrome OS so it's not an issue for other platforms that have OOP-D on in finch trial.

The compositor gets begin frames that contain the vsync interval (which is equivalent to the GPU refresh rate). That could be used to keep |refresh_rate_| up to date, it's just a matter of intercepting the OnBeginFrame() message from the LayerTreeFrameSinks BeginFrameSource.
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c62990ade547e536439b34a278c6369ef1c8582

commit 7c62990ade547e536439b34a278c6369ef1c8582
Author: kylechar <kylechar@chromium.org>
Date: Tue Oct 30 16:53:54 2018

Remove --limit-fps flag.

The flag was added for experimenting with power usage and doesn't appear
used anymore. It's also broken with OOP-D enabled.

Bug:  899300 
Change-Id: Id255ce74d252f15346affb34417b883b37fe5987
Reviewed-on: https://chromium-review.googlesource.com/c/1305244
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603929}
[modify] https://crrev.com/7c62990ade547e536439b34a278c6369ef1c8582/ui/compositor/compositor.cc
[modify] https://crrev.com/7c62990ade547e536439b34a278c6369ef1c8582/ui/compositor/compositor.h
[modify] https://crrev.com/7c62990ade547e536439b34a278c6369ef1c8582/ui/compositor/compositor_switches.cc
[modify] https://crrev.com/7c62990ade547e536439b34a278c6369ef1c8582/ui/compositor/compositor_switches.h
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d96169b45c11e918f289698cf2836dca9579c262

commit d96169b45c11e918f289698cf2836dca9579c262
Author: kylechar <kylechar@chromium.org>
Date: Fri Nov 02 00:55:00 2018

Fix ui::Compositor::refresh_rate() with OOP-D.

With OOP-D enabled Compositor::SetDisplayVSyncParameters() won't
necessarily be called anymore as most of that logic moves to the GPU
process. This means that |refresh_rate_| won't be updated ever and would
be incorrect.

The vsync interval in received BeginFrameArgs is equivalent to the
refresh rate, so plumb that information out of cc::Scheduler and back
into ui::Compositor. This should always work correctly.

This CL also removes ContextFactory::GetRefreshRate(). This set the
initial value for |refresh_rate_| in tests. This no longer serves any
purpose, as |refresh_rate_| is only used for metric reporting that
doesn't impact tests. Tests that want to run faster already setup a
BeginFrameSource with the appropriate shorter interval.

Bug:  899300 
Change-Id: I8f7dcbb984a4cd804712d39a571e43ad627c9d4b
Reviewed-on: https://chromium-review.googlesource.com/c/1311077
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604783}
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/scheduler/scheduler.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/scheduler/scheduler.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/scheduler/scheduler_unittest.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/trees/layer_tree_host_single_thread_client.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/trees/proxy_impl.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/trees/single_thread_proxy.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/cc/trees/single_thread_proxy.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/gpu_process_transport_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/test/test_image_transport_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/test/test_image_transport_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/viz_process_transport_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/content/browser/compositor/viz_process_transport_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/services/ws/host_context_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/services/ws/host_context_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/aura/mus/mus_context_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/aura/mus/mus_context_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/aura/test/aura_test_context_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/compositor.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/compositor.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/test/fake_context_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/test/fake_context_factory.h
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/test/in_process_context_factory.cc
[modify] https://crrev.com/d96169b45c11e918f289698cf2836dca9579c262/ui/compositor/test/in_process_context_factory.h

Comment 3 by kylec...@chromium.org, Nov 2

Status: Fixed (was: Assigned)
►

Sign in to add a comment