Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626413002:
Route D3D VSync signal to Compositor (Closed)
|
Created:
3 years, 11 months ago by stanisc Modified:
3 years, 10 months ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, piman+watch_chromium.org, scheduler-bugs_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRoute D3D VSync signal to Compositor
This change introduces a new type of BeginFrameSource that listens to
accurate VSync signals generated on GPU side and delivered via
existing GpuCommandBufferMsg_UpdateVSyncParameters IPC.
This provides noticeably less VSync timing variation and ~3x less
latency than the existing delay based mechanism.
List of included changes:
1) GpuVSyncBeginFrameSource - new type of BFS derives from from
ExternalBeginFrameSource and triggers OnBeginFrame promptly when
it receives an external signal.
GpuVSyncBeginFrameSource also includes a VSync control
mechanism to signal whether VSync production need to be started or
stopped on GPU side.
2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl
get a new method to start / stop VSync production on GPU side.
3) GpuProcessTransportFactory decides which type of BeginFrameSource
to create based on a combination of type of output surface, OS,
OS version and whether or not D3D VSync feature is enabled.
4) Based on previous feedback PerCompositorData now supports either
Synthetic BFS or GPU VSync which are mutually exclusive.
VSync parameters used by Synthetic BSF and VSync signal used by
Gpu VSync BFS are delivered using exactly the same codepath and
initiated from the same IPC call. The payload is also the same in
both cases - VSync timestamp and interval.
5) On GPU side this feature is activated by instantiating
GpuVSyncProviderWin instead of VSyncProviderWin. The new provider
is hosted exactly the same way as other providers but it uses a
different mechanism to deliver VSync signal.
See https://codereview.chromium.org/2681033011 for more details.
This feature is disabled by default and might be turned on by
--enable-features=D3DVsync switch or by configuring an experiment
that enables D3DVsync feature.
Please note that this feature is currently disabled on Win7 where
the implementation of GpuVSyncProviderWin seems to be grabbing an
internal D3D lock while waiting for VSync and that significantly
slows down all other D3D calls. I plan to address this in a feature
patch.
BUG=467617
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2626413002
Cr-Commit-Position: refs/heads/master@{#452249}
Committed: https://chromium.googlesource.com/chromium/src/+/4950173d4782941bd49f90be89c1d77422e8ca73
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed CR feedback #
Total comments: 21
Patch Set 3 : Addressed CR feedback (on GPU side) #Patch Set 4 : Implement GPU VSync provider as gl::VSyncProvider #
Total comments: 3
Patch Set 5 : Limit GPU VSync to Win8+ #
Total comments: 10
Patch Set 6 : Addressed CR feedback #Patch Set 7 : Merged with recent BeginFrameSource changes #Messages
Total messages: 60 (36 generated)
Description was changed from ========== VSync_4 initial change BUG= ========== to ========== VSync_4 initial change BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== VSync_4 initial change BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== VSync_4 initial change BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== VSync_4 initial change BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. 3) Whether it is OK to derive the new BeginFrameSource class from SyntheticBeginFrameSource. It looks like it should be derived from ExternalBeginFrameSource but that would make it quite a bit more difficult to integrate it with the rest of GpuProcessTransportFactory code. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
stanisc@chromium.org changed reviewers: + danakj@chromium.org, jbauman@chromium.org, sunnyps@chromium.org
sunnyps@chromium.org: Please review changes in begin_frame_source.h and the new BeginFrameSource type jbauman@chromium.org: Please review changes in gpu/ and ui/gl danakj@chromium.org: Please review changes in content/browser/compositor Feel free to add other reviewers if necessary
Description was changed from ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. 3) Whether it is OK to derive the new BeginFrameSource class from SyntheticBeginFrameSource. It looks like it should be derived from ExternalBeginFrameSource but that would make it quite a bit more difficult to integrate it with the rest of GpuProcessTransportFactory code. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. 3) Whether it is OK to derive the new BeginFrameSource class from SyntheticBeginFrameSource. It looks like it should be derived from ExternalBeginFrameSource but that would make it quite a bit more difficult to integrate it with the rest of GpuProcessTransportFactory code. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
enne@chromium.org changed reviewers: + enne@chromium.org
This seems fine in general to me, other than the begin frame source type issues you mention. I think your new type should really derive from external begin frame source. I think what you could do is have PerCompositorData own both a SyntheticBeginFrameSource and a BeginFrameSource (generic) only one of which is valid at any one point. Then, SetAuthoritativeVsyncInternal and SetDisplayVSyncParameters would only touch the SyntheticBeginFrameSource. This is what mus does already, except it just lets the mus output surface own the begin frame source instead of the PerCompositorData. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_browser_compositor_output_surface.h (right): https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:79: GpuVSyncControl* GetVSyncControl() override; I don't know that this really needs to be part of the interface here. It seems like you can derive from GpuVSyncControl and then just grab this inside of GpuProcessTransportFactory when the GpuBrowserCompositorOutputSurface is created. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:82: void EnableVSync(bool enabled) override; Similarly, EnableVSync doesn't need to be virtual either. I don't think that's something any other BrowserCompositorOutputSurface will handle. Also, is there some better name than EnableVSync? It's not like we're ignoring vsync in other cases?
enne@, thank you for the review! I had this new BFS type derived from ExternalBeginFrameSource as you suggested earlier. While BFS implementation was simpler, I was concerned about the factory code that seemed awkward, more complex than it is now. Recently there was some refactoring in this code that made PerCompositorData owner of BFS so now this approach with having two alternative types of BFS in PerCompositorData might work better. Still that would require any code that references begin_frame_source_ to check both members corresponding to two types of BFS instead. I'll give it another try. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_browser_compositor_output_surface.h (right): https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:79: GpuVSyncControl* GetVSyncControl() override; On 2017/01/13 21:14:11, enne wrote: > I don't know that this really needs to be part of the interface here. It seems > like you can derive from GpuVSyncControl and then just grab this inside of > GpuProcessTransportFactory when the GpuBrowserCompositorOutputSurface is > created. This was my attempt to simplify the logic in the factory code which creates 7 different types of surface derived from BrowserCompositorOutputSurface. Some of them would support this new VSync mechanism, and only on OS_WIN, and some won't. And to make it more complex all newly created instances are immediately assigned to unique_ptr<BrowserCompositorOutputSurface>. The alternative that I've tried is to have some code that will assign GpuVSyncControl* in some (3?) of those 7 cases and then the outcome would have to be different depending on whether it was on OS_WIN or not. And because on creation these instances are assigned to the base class unique_ptr I needed to either downcast it or use an intermediate GpuBrowserCompositorOutputSurface* pointer. So the code didn't look pretty. It is far more straightforward this way. I'll give it another shot. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:82: void EnableVSync(bool enabled) override; On 2017/01/13 21:14:11, enne wrote: > Similarly, EnableVSync doesn't need to be virtual either. I don't think that's > something any other BrowserCompositorOutputSurface will handle. Also, is there > some better name than EnableVSync? It's not like we're ignoring vsync in other > cases? Well, I guess if GpuBrowserCompositorOutputSurface* was passed to GpuVSyncBeginFrameSource I could avoid having this method virtua (and needing GpuVsyncControl interface). I thought that decoupling BFS from the actual implementation of sending Enable/Disable VSync was a cleaner approach. Regarding the name, I agree, I've already realized that that would conflict with a notion of disabling VSync completely via --disable-vsync flag. How about SetNeedsVSync which is consistent with other similar named methods (SetNeedsCommit, SetNeedsRedraw, etc)?
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. 3) Whether it is OK to derive the new BeginFrameSource class from SyntheticBeginFrameSource. It looks like it should be derived from ExternalBeginFrameSource but that would make it quite a bit more difficult to integrate it with the rest of GpuProcessTransportFactory code. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The followed changes are included: 1) The new BeginFrameSource is now derived from ExternalBeginFrameSource. 2) PerCompositorData handles two possible types of BFS (Synthetic BFS and GPU VSync BFS) and has one member field for each. 3) I got rid of GetVSyncControl function. Whether a particular type of output surface supports GPU VSync or not is now coded in GpuProcessTransportFactory. 4) Renamed EnableVSync to SetNeedsVSync everywhere. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... File content/browser/compositor/gpu_browser_compositor_output_surface.h (right): https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:79: GpuVSyncControl* GetVSyncControl() override; On 2017/01/14 00:32:20, stanisc wrote: > On 2017/01/13 21:14:11, enne wrote: > > I don't know that this really needs to be part of the interface here. It > seems > > like you can derive from GpuVSyncControl and then just grab this inside of > > GpuProcessTransportFactory when the GpuBrowserCompositorOutputSurface is > > created. > > This was my attempt to simplify the logic in the factory code which creates 7 > different types of surface derived from BrowserCompositorOutputSurface. Some of > them would support this new VSync mechanism, and only on OS_WIN, and some won't. > And to make it more complex all newly created instances are immediately assigned > to unique_ptr<BrowserCompositorOutputSurface>. The alternative that I've tried > is to have some code that will assign GpuVSyncControl* in some (3?) of those 7 > cases and then the outcome would have to be different depending on whether it > was on OS_WIN or not. And because on creation these instances are assigned to > the base class unique_ptr I needed to either downcast it or use an intermediate > GpuBrowserCompositorOutputSurface* pointer. So the code didn't look pretty. It > is far more straightforward this way. I'll give it another shot. OK, I've removed this function. https://codereview.chromium.org/2626413002/diff/1/content/browser/compositor/... content/browser/compositor/gpu_browser_compositor_output_surface.h:82: void EnableVSync(bool enabled) override; On 2017/01/14 00:32:20, stanisc wrote: > On 2017/01/13 21:14:11, enne wrote: > > Similarly, EnableVSync doesn't need to be virtual either. I don't think > that's > > something any other BrowserCompositorOutputSurface will handle. Also, is > there > > some better name than EnableVSync? It's not like we're ignoring vsync in other > > cases? > > Well, I guess if GpuBrowserCompositorOutputSurface* was passed to > GpuVSyncBeginFrameSource I could avoid having this method virtua (and needing > GpuVsyncControl interface). I thought that decoupling BFS from the actual > implementation of sending Enable/Disable VSync was a cleaner approach. > > Regarding the name, I agree, I've already realized that that would conflict with > a notion of disabling VSync completely via --disable-vsync flag. How about > SetNeedsVSync which is consistent with other similar named methods > (SetNeedsCommit, SetNeedsRedraw, etc)? I've kept this as virtual for now, mostly for the purpose of decoupling GpuVSyncControl facet of functionality from the rest of GpuBrowserCompositorOutputSurface. I could get rid of GpuVSyncControl if I made GpuVsyncBeginFrameSource depend directly on this class. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:15: class GpuVSyncControl { Should this be moved to the same header file with GpuBrowserOutputSurface? https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:22: class GpuVSyncBeginFrameSource : public cc::ExternalBeginFrameSource, Since this class is so simple now should it be moved inline into gpu_process_transport_factory.cc?
Thanks. I like this a lot better. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_browser_compositor_output_surface.cc:47: void GpuBrowserCompositorOutputSurface::SetNeedsVSync(bool needs_vsync) { I still think this is a little bit confusing of a name. Even when this is false, the command buffer sends (different) vsync info back to the browser. Do you have some way of differentiating this "needs vsync" from the general "needs vsync"? Thinking out loud, is this "needs d3d vsync"? Or "needs vsync events" (vs info)? https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.h (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:15: class GpuVSyncControl { On 2017/01/24 at 21:23:25, stanisc wrote: > Should this be moved to the same header file with GpuBrowserOutputSurface? Yeah, that seems reasonable. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.h:22: class GpuVSyncBeginFrameSource : public cc::ExternalBeginFrameSource, On 2017/01/24 at 21:23:25, stanisc wrote: > Since this class is so simple now should it be moved inline into gpu_process_transport_factory.cc? I don't feel strongly. I think it's fine to leave it as a separate file.
https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_browser_compositor_output_surface.cc:47: void GpuBrowserCompositorOutputSurface::SetNeedsVSync(bool needs_vsync) { On 2017/01/24 21:32:07, enne wrote: > I still think this is a little bit confusing of a name. Even when this is > false, the command buffer sends (different) vsync info back to the browser. Do > you have some way of differentiating this "needs vsync" from the general "needs > vsync"? > > Thinking out loud, is this "needs d3d vsync"? Or "needs vsync events" (vs info)? When D3D VSync feature is enabled that fully replaces the old vsync parameters mechanism. On the GPU side VSyncProvider is still invoked but only to get the vsync interval which on windows comes from WDM, but IPC calls are not triggered by the old mechanism anymore. We could get rid of the old mechanism entirely if we find an accurate way to get vsync intervals. We could probably just derive them from consecutive vsync events but I haven't implemented that yet.
Thanks, this is looking pretty good. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:231: base::FieldTrialList::FindFullName("UseD3DVSync"); Could you use base::Feature here instead? https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:418: void GpuCommandBufferStub::SendUpdatedVSyncParameters( Technically Send on an IPC::SyncChannel can only be called from the main thread. You'll need to create a SyncMessageFilter using IPC::SyncChannel::CreateSyncMessageFilter and send on that. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:688: vsync_provider_ = GpuVSyncProvider::Create( Only create this for stubs that aren't "offscreen". https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:831: DCHECK(vsync_provider_); if (!vsync_provider_) return; This message could be sent from an untrusted renderer.
I think there's a lot of confusion on the gpu process side because of two kinds of vsync providers. I'd really like to not have two kinds of vsync providers which are both alive in different places at the same time and do different things. We should really try to combine the two interfaces. Here's what I would do: 1. Add an EnableGpuVSync(callback) (and DisableGpuVSync) method to VSyncProvider. 2. Make all vsync provider implementations noop on this, or NOTIMPLEMENTED whichever makes more sense. 3. Merge gpu_vsync_provider_win with vsync_provider_win. 4. Set the EnableGpuVSync callback to GpuCommandBufferStub::SendUpdateVSyncParameters (if gpu preferences gpu vsync is enabled) 5. Make GpuCommandBufferStub::UpdateVSyncParameters (called from image transport surface) noop if gpu vsync is enabled. 6. Get the timebase/interval via VSyncProviderWin::GetVSyncParameters on the background thread. If that's not possible you can cache the timebase/interval in the vsync provider instead of the command buffer. There are possibly lots of other ways to get the same result of avoiding confusion. I wouldn't mind another type of IPC message to differentiate between vsync param updates and vsync but that might involve more plumbing on the client side. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:115: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); This belongs in content/browser/gpu/compositor_util. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:790: if (data->synthetic_begin_frame_source) nit: braces https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.cc (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.cc:31: next_sequence_number_++; I think you want to increment seq num after begin frame. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:224: bool IsGpuVSyncSignalSupported() { This should be passed from the browser to gpu process via GpuPreferences.
@sunnyps, thank you for your feedback. I have certainly considered implementing this as a true VSyncProvider and my early version of the prototype did just that. The main issue with that approach is VSyncProvider is hosted by at ImageTransportSurface level and that it pretty well isolated from GpuCommandBufferStub which makes it hard if not impossible to invoke VSync IPC on the worker thread. I really want to avoid having to go through the main GPU thread because it is already a bottleneck and doing that will likely add latency. When ImageTransportSurface is constructed it gets WeakPtr<ImageTransportSurfaceDelegate> which is basically the only indirect reference to GpuCommandBufferStub. And that WeakPtr must be dereferenced only on the main tread. I guess if I added a way for ImageTransportSurface to create an IPC SyncMessageFilter then perhaps I could use that as a way to send IPC messages on the worker thread as jbauman@ suggested - either do that directly on ImageTransportSurface level or pass the filter to the new type of VSyncProvider. But either way that isn't straightforward with the current design. Another way to reduce the confusion is to rename GpuVSyncProvider to something like VSyncSignalProvider or perhaps rename the old VSyncProvider to something like VSyncParameterProvider. Technically these two types of providers provide different things - the old VSyncProvider provides arguments for delay based VSync generation and it doesn't have to do that on every single frame. The new one provides actual VSync signals. On 2017/01/25 01:36:41, sunnyps wrote: > I think there's a lot of confusion on the gpu process side because of two kinds > of vsync providers. I'd really like to not have two kinds of vsync providers > which are both alive in different places at the same time and do different > things. We should really try to combine the two interfaces. > > Here's what I would do: > > 1. Add an EnableGpuVSync(callback) (and DisableGpuVSync) method to > VSyncProvider. > 2. Make all vsync provider implementations noop on this, or NOTIMPLEMENTED > whichever makes more sense. > 3. Merge gpu_vsync_provider_win with vsync_provider_win. > 4. Set the EnableGpuVSync callback to > GpuCommandBufferStub::SendUpdateVSyncParameters (if gpu preferences gpu vsync is > enabled) > 5. Make GpuCommandBufferStub::UpdateVSyncParameters (called from image transport > surface) noop if gpu vsync is enabled. > 6. Get the timebase/interval via VSyncProviderWin::GetVSyncParameters on the > background thread. If that's not possible you can cache the timebase/interval in > the vsync provider instead of the command buffer. > > There are possibly lots of other ways to get the same result of avoiding > confusion. > > I wouldn't mind another type of IPC message to differentiate between vsync param > updates and vsync but that might involve more plumbing on the client side.
I've addressed some of CR feedback. I am still going to look whether it is possible to implement or wrap GpuSyncProvider as a special type of the old style SyncProvider which would make it hosted by PassThroughImageTransportSurface. I guess that could be possible if I surfaced a function to create SyncMessageFilter at ImageTransportSurfaceDelegate and used the filter to send the IPC message directly from the sync provider and ignored the callback. I am not sure if the same filter could be used to receive SetNeedsVSync IPC calls from the browser - that would be quite convenient. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:115: base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); On 2017/01/25 01:36:41, sunnyps wrote: > This belongs in content/browser/gpu/compositor_util. This is a temporary switch which I hope to get rid of once the new VSync mechanism gets well tested and stable. Considering that this function is trivial (especially with base::FeatureList implementation) and applies only inside this file I think it makes more sense to keep it here. https://codereview.chromium.org/2626413002/diff/20001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:790: if (data->synthetic_begin_frame_source) On 2017/01/25 01:36:41, sunnyps wrote: > nit: braces Done. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... File gpu/ipc/service/gpu_command_buffer_stub.cc (right): https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:224: bool IsGpuVSyncSignalSupported() { On 2017/01/25 01:36:41, sunnyps wrote: > This should be passed from the browser to gpu process via GpuPreferences. I think it is doesn't make much sense to over-complicate this considering that this switch is temporary and should be removed later. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:231: base::FieldTrialList::FindFullName("UseD3DVSync"); On 2017/01/25 00:00:16, jbauman wrote: > Could you use base::Feature here instead? Done. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:418: void GpuCommandBufferStub::SendUpdatedVSyncParameters( On 2017/01/25 00:00:16, jbauman wrote: > Technically Send on an IPC::SyncChannel can only be called from the main thread. > You'll need to create a SyncMessageFilter using > IPC::SyncChannel::CreateSyncMessageFilter and send on that. Done. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:688: vsync_provider_ = GpuVSyncProvider::Create( On 2017/01/25 00:00:16, jbauman wrote: > Only create this for stubs that aren't "offscreen". Done. https://codereview.chromium.org/2626413002/diff/20001/gpu/ipc/service/gpu_com... gpu/ipc/service/gpu_command_buffer_stub.cc:831: DCHECK(vsync_provider_); On 2017/01/25 00:00:16, jbauman wrote: > if (!vsync_provider_) return; > > This message could be sent from an untrusted renderer. Done.
Description was changed from ========== Route D3D VSync signal to Compositor This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by either --use-d3d-vsync switch or an experiment. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gl::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. It doesn't however uses the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gl::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gl::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. It doesn't however uses the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gl::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gfx::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. It doesn't however uses the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gfx::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
See updated issue description and remained issues. https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/gpu_vsy... File gpu/ipc/service/gpu_vsync_provider_win.h (right): https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/gpu_vsy... gpu/ipc/service/gpu_vsync_provider_win.h:31: // TODO: this should be WIN only Forgot to remove this comment. Will remove it later and replace with the class description. https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/image_t... File gpu/ipc/service/image_transport_surface_win.cc (right): https://codereview.chromium.org/2626413002/diff/60001/gpu/ipc/service/image_t... gpu/ipc/service/image_transport_surface_win.cc:37: vsync_provider.reset(new GpuVSyncProviderWin(delegate, surface_handle)); This doesn't work for CreateViewGLSurface case below. Should I add a WIN_OS only overload of CreateViewGLSurface that takes VSyncProvider?
Description was changed from ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gfx::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. It doesn't however uses the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gfx::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gfx::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. However it doesn't use the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gfx::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
I'd like to better understand how this will work with Mus. Please don't add compositing things to content/ components/display_compositor is currently preferred. How does this vsync signal make its way into the browser? Thanks! https://codereview.chromium.org/2626413002/diff/60001/content/browser/composi... File content/browser/compositor/gpu_vsync_begin_frame_source.cc (right): https://codereview.chromium.org/2626413002/diff/60001/content/browser/composi... content/browser/compositor/gpu_vsync_begin_frame_source.cc:9: GpuVSyncBeginFrameSource::GpuVSyncBeginFrameSource( Does this need to live in content? Can this live in cc somewhere or components/display_compositor?
On 2017/02/14 21:52:30, Fady Samuel wrote: > I'd like to better understand how this will work with Mus. Please don't add > compositing things to content/ components/display_compositor is currently > preferred. How does this vsync signal make its way into the browser? Thanks! > > https://codereview.chromium.org/2626413002/diff/60001/content/browser/composi... > File content/browser/compositor/gpu_vsync_begin_frame_source.cc (right): > > https://codereview.chromium.org/2626413002/diff/60001/content/browser/composi... > content/browser/compositor/gpu_vsync_begin_frame_source.cc:9: > GpuVSyncBeginFrameSource::GpuVSyncBeginFrameSource( > Does this need to live in content? Can this live in cc somewhere or > components/display_compositor? Shared internal design doc with you. Basically GPU VSync signal will get to the compositor over the same existing channel currently used to deliver VSync parameters. On GPU process side the signal will be generated by a special new GpuVSyncProvider that will wait for the signal and send it over to the browser process via GpuCommandBufferMsg_UpdateVSyncParameters. On Browser process side this will be delivered to BeginFrameSource just like in the current implementation. The new special type of BeginFrameSource will treat that as an external begin frame signal and just trigger OnBeginFrame accordingly. So basically there is nothing new except the new type of BeginFrameSource plus some logic to decide whether to create this new type of BeginFrameSource or DelayBasedBeginFrameSource.
Description was changed from ========== Route D3D VSync signal to Compositor UPDATE: Based on the CR feedback so far I made the following changes: 1) On the compositor side the new BeginFrameSource type now derives from ExternalBeginFrameSource. GpuProcessTransportFactory deals with the added complexity and supports two types of BeginFrameSource in its PerCompositor data. 2) On the GPU side GpuVSyncProvider is now derived from gfx::VSyncProvider which means it can be hosted transparently at a GL surface just like a regular VSync provider. However it doesn't use the established OnVSyncParametersUpdated callback mechanism and instead implements its own IPC MessageFilter for sending and receiving IPC messages directly bypassing the main GPU thread. REMAINING ISSUES: The current implementation is likely broken on Win7 where Direct Composition isn't supported because it would try to use the new VSync mechanism on the browser side but stick to the old mechanism on the GPU side. The problem is in ImageTransportSurface::CreateNativeSurface that calls gl::init::CreateViewGLSurface(surface_handle) when IsDirectCompositionSupported returns false. In order to solve this I might try to add an overload of CreateViewGLSurface that takes gfx::VSyncProvider as the second argument. Would that be a reasonable approach? PREVIOUS DESCIPTION: This change is still somewhat experimental at the current state. I am looking for a feedback on design and implementation details and based on the feedback potentially substantial changes might be needed. This change leverages recently introduced GpuVSyncProvider (see https://codereview.chromium.org/2596123002/) that generates VSync signals by calling D3DKMTWaitForVerticalBlankEvent on background thread. That signal is then sent over existing IPC (GpuCommandBufferMsg_UpdateVSyncParameters) to the browser where it gets routed to a new type of BeginFrameSource which then uses that as an external trigger for a new frame. There is a new IPC from Browser to GPU to enable/disable VSync production on the GPU side. This provides noticeably less VSync timing variation than the existing delay based mechanism. Things that I am not sure about: 1) Whether it is a good decision to leverage the same IPC method as the one used to pass VSync parameters. It is very convenient because it requires very little new code on the browser side. 2) Where to get VSync interval from. This implementation leverages VSync interval returned by VSyncProvider (while the timebase returned from VSyncProvider is ignored). Two alternatives I can think of are (1) generate the interval from consecutive timestamps which might be less stable and cause video playback smoothness issues or (2) copy relevant parts of VSyncProviderWin code to the new provider and it could generate both timestamps and intervals. Eventually VSyncProviderWin might not be needed. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change introduces a new type of BeginFrameSource that listens to accurate VSync signals generated on GPU side and delivered via existing GpuCommandBufferMsg_UpdateVSyncParameters IPC. This provides noticeably less VSync timing variation and less latency than the existing delay based mechanism. List of included changes: 1) GpuVSyncBeginFrameSource - new type of BFS derives from from ExternalBeginFrameSource and triggers OnBeginFrame promptly when it receives an external signal. GpuVSyncBeginFrameSource also includes a VSync control mechanism to signal whether VSync production need to be started or stopped on GPU side. 2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl get a new method to start / stop VSync production on GPU side. 3) GpuProcessTransportFactory decides which type of BeginFrameSource to create based on a combination of type of output surface, OS, OS version and whether or not D3D VSync feature is enabled. 4) Based on previous feedback PerCompositorData now supports either Synthetic BFS or GPU VSync which are mutually exclusive. VSync parameters used by Synthetic BSF and VSync signal used by Gpu VSync BFS are delivered using exactly the same codepath and initiated from the same IPC call. The payload is also the same in both cases - VSync timestamp and interval. 5) On GPU side this feature is activated by instantiating GpuVSyncProviderWin instead of VSyncProviderWin. The new provider is hosted exactly the same way as other providers but it uses a different mechanism to deliver VSync signal. See https://codereview.chromium.org/2681033011 for more details. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. Please note that this feature is currently disabled on Win7 where the implementation of GpuVSyncProviderWin seems to be grabbing an internal D3D lock while waiting for VSync and that significantly slows down all other D3D calls. I plan to address this in a feature patch. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
I split off and landed some of the the previously reviewed changes as a part of https://codereview.chromium.org/2681033011. The remaining changes are mostly compositor related code to integrate the new BeginFrameSource type plus some code to enable the feature behind a switch (see description). jbauman@ - OWNER for gpu/ and ui/gl danakj@ or jbauman@ - OWNER for content/browser/compositor Other reviewers - feel free to chime in.
c/b/c/ LGTM just a few comments https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... File content/browser/compositor/gpu_browser_compositor_output_surface.h (right): https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_browser_compositor_output_surface.h:80: // GpuVSyncControl Comments are sentences. "GpuVSyncControl implementation." https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:572: if (!compositor->GetRendererSettings().disable_display_vsync) { nit: maybe it's just me but i think it's easier to read generally if the shorter body comes from the if() than the else, so I'd invert this https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:575: new GpuVSyncBeginFrameSource(gpu_vsync_control)); prefer "= MakeUnique<T>" to ".reset(new T" https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:610: // Note that we are careful not to destroy prior begin frame sources BeginFrameSources will be more greppable and search/replaceable https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:792: } else if (data->gpu_vsync_begin_frame_source) if one block of if/else has {} then they all do https://google.github.io/styleguide/cppguide.html#Conditionals
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed CR feedback from danakj@ https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... File content/browser/compositor/gpu_browser_compositor_output_surface.h (right): https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_browser_compositor_output_surface.h:80: // GpuVSyncControl On 2017/02/16 15:56:02, danakj wrote: > Comments are sentences. "GpuVSyncControl implementation." Done. https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:572: if (!compositor->GetRendererSettings().disable_display_vsync) { On 2017/02/16 15:56:02, danakj wrote: > nit: maybe it's just me but i think it's easier to read generally if the shorter > body comes from the if() than the else, so I'd invert this I see your point. The current if() case is predominantly more common because disabling vsync requires a special command line switch. And it was this way before so I am hesitant to change this. https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:575: new GpuVSyncBeginFrameSource(gpu_vsync_control)); On 2017/02/16 15:56:02, danakj wrote: > prefer "= MakeUnique<T>" to ".reset(new T" Done. https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:610: // Note that we are careful not to destroy prior begin frame sources On 2017/02/16 15:56:02, danakj wrote: > BeginFrameSources will be more greppable and search/replaceable Done. https://codereview.chromium.org/2626413002/diff/80001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:792: } else if (data->gpu_vsync_begin_frame_source) On 2017/02/16 15:56:02, danakj wrote: > if one block of if/else has {} then they all do > > https://google.github.io/styleguide/cppguide.html#Conditionals Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Route D3D VSync signal to Compositor This change introduces a new type of BeginFrameSource that listens to accurate VSync signals generated on GPU side and delivered via existing GpuCommandBufferMsg_UpdateVSyncParameters IPC. This provides noticeably less VSync timing variation and less latency than the existing delay based mechanism. List of included changes: 1) GpuVSyncBeginFrameSource - new type of BFS derives from from ExternalBeginFrameSource and triggers OnBeginFrame promptly when it receives an external signal. GpuVSyncBeginFrameSource also includes a VSync control mechanism to signal whether VSync production need to be started or stopped on GPU side. 2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl get a new method to start / stop VSync production on GPU side. 3) GpuProcessTransportFactory decides which type of BeginFrameSource to create based on a combination of type of output surface, OS, OS version and whether or not D3D VSync feature is enabled. 4) Based on previous feedback PerCompositorData now supports either Synthetic BFS or GPU VSync which are mutually exclusive. VSync parameters used by Synthetic BSF and VSync signal used by Gpu VSync BFS are delivered using exactly the same codepath and initiated from the same IPC call. The payload is also the same in both cases - VSync timestamp and interval. 5) On GPU side this feature is activated by instantiating GpuVSyncProviderWin instead of VSyncProviderWin. The new provider is hosted exactly the same way as other providers but it uses a different mechanism to deliver VSync signal. See https://codereview.chromium.org/2681033011 for more details. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. Please note that this feature is currently disabled on Win7 where the implementation of GpuVSyncProviderWin seems to be grabbing an internal D3D lock while waiting for VSync and that significantly slows down all other D3D calls. I plan to address this in a feature patch. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change introduces a new type of BeginFrameSource that listens to accurate VSync signals generated on GPU side and delivered via existing GpuCommandBufferMsg_UpdateVSyncParameters IPC. This provides noticeably less VSync timing variation and ~3x less latency than the existing delay based mechanism. List of included changes: 1) GpuVSyncBeginFrameSource - new type of BFS derives from from ExternalBeginFrameSource and triggers OnBeginFrame promptly when it receives an external signal. GpuVSyncBeginFrameSource also includes a VSync control mechanism to signal whether VSync production need to be started or stopped on GPU side. 2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl get a new method to start / stop VSync production on GPU side. 3) GpuProcessTransportFactory decides which type of BeginFrameSource to create based on a combination of type of output surface, OS, OS version and whether or not D3D VSync feature is enabled. 4) Based on previous feedback PerCompositorData now supports either Synthetic BFS or GPU VSync which are mutually exclusive. VSync parameters used by Synthetic BSF and VSync signal used by Gpu VSync BFS are delivered using exactly the same codepath and initiated from the same IPC call. The payload is also the same in both cases - VSync timestamp and interval. 5) On GPU side this feature is activated by instantiating GpuVSyncProviderWin instead of VSyncProviderWin. The new provider is hosted exactly the same way as other providers but it uses a different mechanism to deliver VSync signal. See https://codereview.chromium.org/2681033011 for more details. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. Please note that this feature is currently disabled on Win7 where the implementation of GpuVSyncProviderWin seems to be grabbing an internal D3D lock while waiting for VSync and that significantly slows down all other D3D calls. I plan to address this in a feature patch. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
jbauman@, could you review remaining files under gpu/ipc and ui/gl?
gpu/ and ui/gl lgtm
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2626413002/#ps120001 (title: "Merged with recent BeginFrameSource changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
stanisc@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Need OWNER approval for content/browser/BUILD.gn
build lgtm
The CQ bit was checked by stanisc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487803458037320,
"parent_rev": "9db361d1fbd4504e57d479a1213ce363d4859d0f", "commit_rev":
"4950173d4782941bd49f90be89c1d77422e8ca73"}
Message was sent while issue was closed.
Description was changed from ========== Route D3D VSync signal to Compositor This change introduces a new type of BeginFrameSource that listens to accurate VSync signals generated on GPU side and delivered via existing GpuCommandBufferMsg_UpdateVSyncParameters IPC. This provides noticeably less VSync timing variation and ~3x less latency than the existing delay based mechanism. List of included changes: 1) GpuVSyncBeginFrameSource - new type of BFS derives from from ExternalBeginFrameSource and triggers OnBeginFrame promptly when it receives an external signal. GpuVSyncBeginFrameSource also includes a VSync control mechanism to signal whether VSync production need to be started or stopped on GPU side. 2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl get a new method to start / stop VSync production on GPU side. 3) GpuProcessTransportFactory decides which type of BeginFrameSource to create based on a combination of type of output surface, OS, OS version and whether or not D3D VSync feature is enabled. 4) Based on previous feedback PerCompositorData now supports either Synthetic BFS or GPU VSync which are mutually exclusive. VSync parameters used by Synthetic BSF and VSync signal used by Gpu VSync BFS are delivered using exactly the same codepath and initiated from the same IPC call. The payload is also the same in both cases - VSync timestamp and interval. 5) On GPU side this feature is activated by instantiating GpuVSyncProviderWin instead of VSyncProviderWin. The new provider is hosted exactly the same way as other providers but it uses a different mechanism to deliver VSync signal. See https://codereview.chromium.org/2681033011 for more details. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. Please note that this feature is currently disabled on Win7 where the implementation of GpuVSyncProviderWin seems to be grabbing an internal D3D lock while waiting for VSync and that significantly slows down all other D3D calls. I plan to address this in a feature patch. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Route D3D VSync signal to Compositor This change introduces a new type of BeginFrameSource that listens to accurate VSync signals generated on GPU side and delivered via existing GpuCommandBufferMsg_UpdateVSyncParameters IPC. This provides noticeably less VSync timing variation and ~3x less latency than the existing delay based mechanism. List of included changes: 1) GpuVSyncBeginFrameSource - new type of BFS derives from from ExternalBeginFrameSource and triggers OnBeginFrame promptly when it receives an external signal. GpuVSyncBeginFrameSource also includes a VSync control mechanism to signal whether VSync production need to be started or stopped on GPU side. 2) GpuBrowserCompositorOutputSurface and CommandBufferProxyImpl get a new method to start / stop VSync production on GPU side. 3) GpuProcessTransportFactory decides which type of BeginFrameSource to create based on a combination of type of output surface, OS, OS version and whether or not D3D VSync feature is enabled. 4) Based on previous feedback PerCompositorData now supports either Synthetic BFS or GPU VSync which are mutually exclusive. VSync parameters used by Synthetic BSF and VSync signal used by Gpu VSync BFS are delivered using exactly the same codepath and initiated from the same IPC call. The payload is also the same in both cases - VSync timestamp and interval. 5) On GPU side this feature is activated by instantiating GpuVSyncProviderWin instead of VSyncProviderWin. The new provider is hosted exactly the same way as other providers but it uses a different mechanism to deliver VSync signal. See https://codereview.chromium.org/2681033011 for more details. This feature is disabled by default and might be turned on by --enable-features=D3DVsync switch or by configuring an experiment that enables D3DVsync feature. Please note that this feature is currently disabled on Win7 where the implementation of GpuVSyncProviderWin seems to be grabbing an internal D3D lock while waiting for VSync and that significantly slows down all other D3D calls. I plan to address this in a feature patch. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2626413002 Cr-Commit-Position: refs/heads/master@{#452249} Committed: https://chromium.googlesource.com/chromium/src/+/4950173d4782941bd49f90be89c1... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4950173d4782941bd49f90be89c1... |
