New issue
Advanced search Search tips

Issue 885169 link

Starred by 1 user
Status: Fixed
Owner:
kylec...@chromium.org
Closed: Sep 18
Cc:
danakj@chromium.org
rjkroege@chromium.org
sgilhuly@chromium.org
afakhry@chromium.org
osh...@chromium.org
spang@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

ContextFactoryPrivate::SetAuthoritativeVSyncInterval() isn't necessary.

Project Member Reported by kylec...@chromium.org, Sep 18 
SetAuthoritativeVSyncInterval() isn't doing anything useful anymore. The function is only used on Chrome OS. It sets an authoritative vsync interval based on the refresh rate Ozone DRM tells it. This is the same vsync interval that Ozone DRM returns through the command buffer. SetAuthoritativeVSyncInterval() seems to be left over from X11 Chrome OS where XrandR would return a more accurate refresh rate than the GL context.

In addition to not being necessary, the first call to GpuProcessTransportFactory::SetAuthoritativeVSyncInterval() on startup or when a display is added happens before |per_compositor_data_| has an entry for the compositor. It takes the early out here:

https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_transport_factory.cc?l=849&rcl=450d772cac4a3f1306eeaf60f941135f327247ba

SetAuthoritativeVSyncInterval() will only get called again if the user changes the display configuration after the display is added, so most of the time the authoritative interval isn't even set.

I don't see any reason to not remove the function?

Comment 1 by kylec...@chromium.org, Sep 18

Components: Internals>Compositing

Comment 2 by kylec...@chromium.org, Sep 18

Cc: afakhry@chromium.org

Comment 3 by danakj@chromium.org, Sep 18 

Thanks for spotting this.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 18 

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

commit f305d23e180a2d68fce56ceb1aac928a0dcbda74
Author: kylechar <kylechar@chromium.org>
Date: Tue Sep 18 19:42:49 2018

Remove SetAuthoritativeVsyncInterval().

The function is only used on Chrome OS and is no longer necessary. Ozone
DRM provides display refresh rates and vsync interval through the
command buffer. The function seems to be leftover from X11 Chrome OS
where Xrandr provided better refresh rate data than the GL context.

GpuProcessTransportFactory::SetAuthoritativeVsyncInterval() is also
broken currently and not setting the interval.

Bug:  885169 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I66ffa73f5b32418cd83553c52ff97293e1b10657
Reviewed-on: https://chromium-review.googlesource.com/1230578
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592151}
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ash/display/window_tree_host_manager.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/common/frame_sinks/begin_frame_source.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/common/frame_sinks/begin_frame_source.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/common/frame_sinks/begin_frame_source_unittest.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/service/frame_sinks/root_compositor_frame_sink_impl.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/components/viz/test/test_layer_tree_frame_sink.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/content/browser/compositor/gpu_process_transport_factory.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/content/browser/compositor/test/test_image_transport_factory.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/services/viz/privileged/interfaces/compositing/display_private.mojom
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/compositor.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/compositor.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/compositor_vsync_manager.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/compositor_vsync_manager.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/host/host_context_factory_private.cc
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/host/host_context_factory_private.h
[modify] https://crrev.com/f305d23e180a2d68fce56ceb1aac928a0dcbda74/ui/compositor/test/in_process_context_factory.h

Comment 5 by kylec...@chromium.org, Sep 18

Status: Fixed (was: Assigned)
►

Sign in to add a comment