Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511273002:
Decouple BrowserCompositorOutputSurface from BeginFrameSource. (Closed)
|
Created:
4 years, 1 month ago by stanisc Modified:
4 years ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, jam, jbauman+watch_chromium.org, kalyank, miu+watch_chromium.org, piman+watch_chromium.org, scheduler-bugs_chromium.org, Ian Vollick, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDecouple BrowserCompositorOutputSurface from BeginFrameSource.
This change is a part of larger effort of propagating D3D VSync
signal to the compositor. Since the current implementation in
BrowserCompositorOutputSurface explicitly depends on a time based
SyntheticBeginFrameSource, enne@ suggested that a good first step
would be to try to decouple it from a specific BeginFrameSource
type.
Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager
in every constructor of BrowserCompositorOutputSurface and classes
derived from it, this change replaces the SyntheticBFS / VSyncManager
pair with a callback to update VSync parameters. The callback is
handled at GpuProcessTransportFactory.
BUG=467617
Committed: https://crrev.com/3566f880e80d65a57b7e084ccd55030972585c98
Cr-Commit-Position: refs/heads/master@{#436122}
Patch Set 1 #Patch Set 2 : Fixed build error in gpu_output_surface_mac.* #Patch Set 3 : Attempt to fix the crash #Patch Set 4 : Another attempt to fix the crash #
Total comments: 4
Patch Set 5 : Changed implementation to pass callback to GPTF #Patch Set 6 : Fixed unit tests #
Total comments: 2
Patch Set 7 : Avoid duplicating code that updates VSync manager. #
Total comments: 9
Patch Set 8 : Addressed CR feedback #
Total comments: 11
Patch Set 9 : Addressed CR feedback #
Total comments: 1
Patch Set 10 : Bring back GpuBrowserCompositorOutputSurface destructor #Messages
Total messages: 70 (42 generated)
Description was changed from ========== Remove BeginFrameSource from output surface. BUG= ========== to ========== Remove BeginFrameSource from output surface. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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.
Description was changed from ========== Remove BeginFrameSource from output surface. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time base SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource in every BrowserCompositorOutputSurface constructor this change makes it an observer to CompositorSyncManager. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
stanisc@chromium.org changed reviewers: + danakj@chromium.org, enne@chromium.org
This might still need a couple of potential refinements - I left a few TODOs in the code to point to potential issues which I am going to resolve after the receiving CR feedback. Issues that I worry about: 1) Propagating of VSync updates seems to be different on MAC_OS. I think some of the existing code might be redundant now that BeginFrameSource is an observer of VSync Manager. 2) Destruction / re-initialization of BeginFrameSource. Initially I had some issues with stale BeginFrameSource left in the observer list in CompositorVSyncManager. I think I resolved those issues in gpu_process_transport_factory.cc but wanted you do double check my solution. See TODOs.
https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:455: compositor->vsync_manager()->AddObserver(begin_frame_source.get()); I think there are at least 3 ways to do this: 1. What you did here exactly. Has a bunch of manual book-keeping about add/remove observer in this class that's easy to mess up. 2. Make Display take a SyntheticBeginFrameSource instead of a BeginFrameSource, have it Add/Remove observer. I think Display take the base-class interface right now because we used to sometimes give it a non-synthethic subclass, but we don't now as BackToBack is synthetic too. Maybe something else would break I'm forgetting? 3. Make a VsyncInformer type interface in cc that SyntheticBFS takes, it uses that to register itself as an observer. CompositorVsyncManager is-a VsyncInformer then. The last 2 have easier to maintain lifetimes around observer-ness. enne@ any preferences or do you prefer 1?
https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:455: compositor->vsync_manager()->AddObserver(begin_frame_source.get()); On 2016/11/21 22:32:11, danakj wrote: > I think there are at least 3 ways to do this: > > 1. What you did here exactly. Has a bunch of manual book-keeping about > add/remove observer in this class that's easy to mess up. > 2. Make Display take a SyntheticBeginFrameSource instead of a BeginFrameSource, > have it Add/Remove observer. I think Display take the base-class interface > right now because we used to sometimes give it a non-synthethic subclass, but we > don't now as BackToBack is synthetic too. Maybe something else would break I'm > forgetting? > 3. Make a VsyncInformer type interface in cc that SyntheticBFS takes, it uses > that to register itself as an observer. CompositorVsyncManager is-a > VsyncInformer then. > > The last 2 have easier to maintain lifetimes around observer-ness. enne@ any > preferences or do you prefer 1? Thanks for the suggestion! The idea was that a new BeginFrameSource that receives actual VSync signals from GPU process won't be based on SyntheticBeginFrameSource. So the suggestion #2 might not work. The approach #3 sounds good - let BeginFrameSource to subscribe/unsubscribe itself from VSyncManager. I agree it would be much cleaner. What name for the interface would you prefer - VSyncInformer, VSyncSource, VSyncManager?
https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:455: compositor->vsync_manager()->AddObserver(begin_frame_source.get()); On 2016/11/21 22:52:31, stanisc wrote: > On 2016/11/21 22:32:11, danakj wrote: > > I think there are at least 3 ways to do this: > > > > 1. What you did here exactly. Has a bunch of manual book-keeping about > > add/remove observer in this class that's easy to mess up. > > 2. Make Display take a SyntheticBeginFrameSource instead of a > BeginFrameSource, > > have it Add/Remove observer. I think Display take the base-class interface > > right now because we used to sometimes give it a non-synthethic subclass, but > we > > don't now as BackToBack is synthetic too. Maybe something else would break I'm > > forgetting? > > 3. Make a VsyncInformer type interface in cc that SyntheticBFS takes, it uses > > that to register itself as an observer. CompositorVsyncManager is-a > > VsyncInformer then. > > > > The last 2 have easier to maintain lifetimes around observer-ness. enne@ any > > preferences or do you prefer 1? > > Thanks for the suggestion! The idea was that a new BeginFrameSource that > receives actual VSync signals from GPU process won't be based on > SyntheticBeginFrameSource. So the suggestion #2 might not work. > > The approach #3 sounds good - let BeginFrameSource to subscribe/unsubscribe > itself from VSyncManager. I agree it would be much cleaner. What name for the > interface would you prefer - VSyncInformer, VSyncSource, VSyncManager? Ok so one problem with this approach is that we're going to have to undo it when we move Display out of process. Right now VsyncManager is part of ui::Compositor and used to inform things in the browser process. This is making it also inform the display's BFS which will move to the GPU process. So we're taking backward steps toward an out-of-process Display compositor I think. At a high level I think what we need is to get rid of both the VSyncManager* and the SyntheticBFS* from the BrowserCompositorOutputSurface. We should delegate the receipt of vsync parameters up to a higher level that is going to stay in the browser process and have it tell the VsyncManager. That place can also tell the SyntheticBFS* when it is present, later we'd just delete that and tell the display's SyntheticBFS on the gpu process side before sending that IPC to the browser. The most obvious way to do that is to give a client* or a Callback to the BrowserCompositorOutputSurface that it can call. Make that function a part of GpuProcessTransportFactory, and have it receive the Compositor* as part of its inputs to find the per-compositor-data with the VsyncManager and SyntheticBFS* to update. But I do wonder if GPTF is the right place for this at all. Most of the calls to these things are coming out of ui::Compositor (except for the one from the GPUProcess which is looking more like an outlier to me at this point). So this makes me wonder if we shouldn't instead be listening to vsync info from the ui::Compositor instead and it could pass that info along the same path that it is for Mac currently (https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?rcl=147974512...). And we'd do the same for software compositing.. I'm not totally sure if that's a feasible idea but that seems to be most aligned with where things are going - in that world ui::Compositor would be telling the display compositor out-of-process about vsync stuff on some platforms, and there's no reason to involve GPTF in that anymore at that point.
I think it makes some sense to keep most of this logic in GPTF in the short term, since that's where the Display lives. The stuff going through ui::Compositor isn't really about the ui::Compositor, it's just the only route to say "please update my display's begin frame source" from that part of the code. I also think that the ui::Compositor doesn't know anything about the kinds of updates that happen from the gpu process, and GPTF sits in between these two things right now.
https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/60001/content/browser/composi... content/browser/compositor/gpu_process_transport_factory.cc:455: compositor->vsync_manager()->AddObserver(begin_frame_source.get()); On 2016/11/21 23:06:36, danakj wrote: > On 2016/11/21 22:52:31, stanisc wrote: > > On 2016/11/21 22:32:11, danakj wrote: > > > I think there are at least 3 ways to do this: > > > > > > 1. What you did here exactly. Has a bunch of manual book-keeping about > > > add/remove observer in this class that's easy to mess up. > > > 2. Make Display take a SyntheticBeginFrameSource instead of a > > BeginFrameSource, > > > have it Add/Remove observer. I think Display take the base-class interface > > > right now because we used to sometimes give it a non-synthethic subclass, > but > > we > > > don't now as BackToBack is synthetic too. Maybe something else would break > I'm > > > forgetting? > > > 3. Make a VsyncInformer type interface in cc that SyntheticBFS takes, it > uses > > > that to register itself as an observer. CompositorVsyncManager is-a > > > VsyncInformer then. > > > > > > The last 2 have easier to maintain lifetimes around observer-ness. enne@ any > > > preferences or do you prefer 1? > > > > Thanks for the suggestion! The idea was that a new BeginFrameSource that > > receives actual VSync signals from GPU process won't be based on > > SyntheticBeginFrameSource. So the suggestion #2 might not work. > > > > The approach #3 sounds good - let BeginFrameSource to subscribe/unsubscribe > > itself from VSyncManager. I agree it would be much cleaner. What name for the > > interface would you prefer - VSyncInformer, VSyncSource, VSyncManager? > > Ok so one problem with this approach is that we're going to have to undo it when > we move Display out of process. > > Right now VsyncManager is part of ui::Compositor and used to inform things in > the browser process. This is making it also inform the display's BFS which will > move to the GPU process. So we're taking backward steps toward an out-of-process > Display compositor I think. > > At a high level I think what we need is to get rid of both the VSyncManager* and > the SyntheticBFS* from the BrowserCompositorOutputSurface. We should delegate > the receipt of vsync parameters up to a higher level that is going to stay in > the browser process and have it tell the VsyncManager. That place can also tell > the SyntheticBFS* when it is present, later we'd just delete that and tell the > display's SyntheticBFS on the gpu process side before sending that IPC to the > browser. > > The most obvious way to do that is to give a client* or a Callback to the > BrowserCompositorOutputSurface that it can call. Make that function a part of > GpuProcessTransportFactory, and have it receive the Compositor* as part of its > inputs to find the per-compositor-data with the VsyncManager and SyntheticBFS* > to update. > > But I do wonder if GPTF is the right place for this at all. Most of the calls to > these things are coming out of ui::Compositor (except for the one from the > GPUProcess which is looking more like an outlier to me at this point). So this > makes me wonder if we shouldn't instead be listening to vsync info from the > ui::Compositor instead and it could pass that info along the same path that it > is for Mac currently > (https://cs.chromium.org/chromium/src/ui/compositor/compositor.h?rcl=147974512...). > And we'd do the same for software compositing.. > > I'm not totally sure if that's a feasible idea but that seems to be most aligned > with where things are going - in that world ui::Compositor would be telling the > display compositor out-of-process about vsync stuff on some platforms, and > there's no reason to involve GPTF in that anymore at that point. How about passing a pointer to VSyncObserver to BrowserCompositorOutputSurface instead of CompositorVSyncManager. In this case it might be better to rename VSyncObserver to something like VSyncClient. An object that implements VSyncClient might be owned by PerCompositorData and it would replace a pointer to SyntheticBFS there. The responsibility of this object will be to pass VSync notification to other objects - BFS and VSync Manager. VSyncClient could also be passed to a new type of BFS that I am going to introduce later to allow it to control VSync production on the GPU side.
I think in the future, BCOS can't be a VSyncObserver. I think I would suggest making BCOS not know about either VSyncManager or VSyncObserver or BeginFrameSource and instead have GPTF pass a callback to SetDisplayVSyncParameters, partially bound to the ui::Compositor instance for that BCOS. This would allow both the gpu and the software version to call back to GPTF to update the parameters. Then GPTF would have to do the work of updating VSyncManager and the BeginFrameSource. This consolidates everything in GPTF. In the future, you could pass a different BFS into the Display(Scheduler) as needed and the BCOS wouldn't have to know about it.
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: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Description was changed from ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time base SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource in every BrowserCompositorOutputSurface constructor this change makes it an observer to CompositorSyncManager. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Replaced the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. PTAL!
Thanks! This is looking good. One small nit: https://codereview.chromium.org/2511273002/diff/100001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/100001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:738: SetDisplayVSyncParameters(compositor.get(), timebase, interval); I think UpdateVSyncParametersFromGPU and SetDisplayVSyncParameters are too similar here. Also, "from GPU" isn't really true for the software case. Could you just bind SetDisplayVSyncParameters instead of writing a new function, and have that function update the vsync manager as well? It's probably an oversight that it hasn't done that updating.
https://codereview.chromium.org/2511273002/diff/100001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/100001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:738: SetDisplayVSyncParameters(compositor.get(), timebase, interval); On 2016/11/29 21:25:03, enne wrote: > I think UpdateVSyncParametersFromGPU and SetDisplayVSyncParameters are too > similar here. Also, "from GPU" isn't really true for the software case. > > Could you just bind SetDisplayVSyncParameters instead of writing a new function, > and have that function update the vsync manager as well? It's probably an > oversight that it hasn't done that updating. I tried binding SetDisplayVSyncParameters directly but it expects a regular, non-weak pointer to ui::Compositor. The code that creates the callback has a weak pointer and it doesn't seem to be OK to resolve WeakPtr<ui::Compositor> to ui::Compositor* at that time - it is a weak pointer for a reason, right? So the main reason for this function is to resolve the weak pointer. As for the name I am open for suggestions.
On 2016/11/29 at 21:38:44, stanisc wrote: > So the main reason for this function is to resolve the weak pointer. As for the name I am open for suggestions. That's fair. Then maybe have the function *just* resolve the weak pointer and then call the other function to update the BFS and the vsync manager together. It'd be fine to just call it SetDisplayVSyncParameters as well? Or something similar?
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: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
Changed the GPTF callback to be a non-member function that redirects to Compositor::SetDisplayVSyncParameters. It seems cleaner this way and GPTF doesn't have to know about VSyncManager at all.
lgtm, thanks for all the changes! I think you need a danakj review here as well for OWNERS.
https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_browser_compositor_output_surface.cc:68: &GpuBrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu, This is going to the base class just to call another callback. How about just passing that callback here? Maybe the interval 0 code should move up to the GPTF function? https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:163: if (!compositor) How does the compositor be gone but the output surface is still alive? https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:83: &BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu, This is going to the base class just to call another callback. How about just passing that callback here? Maybe the interval 0 code should move up to the GPTF function?
https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_browser_compositor_output_surface.cc:68: &GpuBrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu, On 2016/11/30 23:49:05, danakj wrote: > This is going to the base class just to call another callback. How about just > passing that callback here? Maybe the interval 0 code should move up to the GPTF > function? Yep, it looks like OnUpdateVSyncParametersFromGpu may be unnecessary anymore. I'll look into that. https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:163: if (!compositor) On 2016/11/30 23:49:05, danakj wrote: > How does the compositor be gone but the output surface is still alive? Honestly I don't know. Why was it WeakPtr<Compositor> in EstablishedGpuChannel the first place? I'll look into this.
https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:163: if (!compositor) On 2016/12/01 00:16:27, stanisc wrote: > On 2016/11/30 23:49:05, danakj wrote: > > How does the compositor be gone but the output surface is still alive? > > Honestly I don't know. Why was it WeakPtr<Compositor> in EstablishedGpuChannel > the first place? > I'll look into this. EstablishedGpuChannel is called from the GpuChannelFactory which is not tied to the lifetime of a compositor in any way. Where as here the callback is from an OutputSurface owned by the Display. The Display is owned by the PerCompositorData which is destroyed in RemoveCompositor which is called from ~Compositor. So it seems to be tied to the lifetime of the Compositor.
On 2016/12/01 00:26:34, danakj wrote: > https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... > File content/browser/compositor/gpu_process_transport_factory.cc (right): > > https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... > content/browser/compositor/gpu_process_transport_factory.cc:163: if > (!compositor) > On 2016/12/01 00:16:27, stanisc wrote: > > On 2016/11/30 23:49:05, danakj wrote: > > > How does the compositor be gone but the output surface is still alive? > > > > Honestly I don't know. Why was it WeakPtr<Compositor> in EstablishedGpuChannel > > the first place? > > I'll look into this. > > EstablishedGpuChannel is called from the GpuChannelFactory which is not tied to > the lifetime of a compositor in any way. Where as here the callback is from an > OutputSurface owned by the Display. The Display is owned by the > PerCompositorData which is destroyed in RemoveCompositor which is called from > ~Compositor. So it seems to be tied to the lifetime of the Compositor. Thanks for explaining! Would this still hold true if we forward the callback further to other objects (command buffer proxy and vsync provider) as suggested in your other two comments. I see WeakPtr to BrowserCompositorOutputSurface is currently used there.
On Wed, Nov 30, 2016 at 5:30 PM, <stanisc@chromium.org> wrote: > On 2016/12/01 00:26:34, danakj wrote: > > > https://codereview.chromium.org/2511273002/diff/110001/ > content/browser/compositor/gpu_process_transport_factory.cc > > File content/browser/compositor/gpu_process_transport_factory.cc > (right): > > > > > https://codereview.chromium.org/2511273002/diff/110001/ > content/browser/compositor/gpu_process_transport_factory.cc#newcode163 > > content/browser/compositor/gpu_process_transport_factory.cc:163: if > > (!compositor) > > On 2016/12/01 00:16:27, stanisc wrote: > > > On 2016/11/30 23:49:05, danakj wrote: > > > > How does the compositor be gone but the output surface is still > alive? > > > > > > Honestly I don't know. Why was it WeakPtr<Compositor> in > EstablishedGpuChannel > > > the first place? > > > I'll look into this. > > > > EstablishedGpuChannel is called from the GpuChannelFactory which is not > tied > to > > the lifetime of a compositor in any way. Where as here the callback is > from an > > OutputSurface owned by the Display. The Display is owned by the > > PerCompositorData which is destroyed in RemoveCompositor which is called > from > > ~Compositor. So it seems to be tied to the lifetime of the Compositor. > > Thanks for explaining! Would this still hold true if we forward the > callback > further > to other objects (command buffer proxy and vsync provider) as suggested in > your > other > two comments. I see WeakPtr to BrowserCompositorOutputSurface is currently > used > there. > Good question. It would if the OutputSurface unset the callback, but not if it left the callback there when it is destroyed. Normally we unset things if we can, cuz explicit is nice. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
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: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_browser_compositor_output_surface.cc:68: &GpuBrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu, On 2016/12/01 00:16:27, stanisc wrote: > On 2016/11/30 23:49:05, danakj wrote: > > This is going to the base class just to call another callback. How about just > > passing that callback here? Maybe the interval 0 code should move up to the > GPTF > > function? > > Yep, it looks like OnUpdateVSyncParametersFromGpu may be unnecessary anymore. > I'll look into that. Done. https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:163: if (!compositor) On 2016/12/01 00:26:34, danakj wrote: > On 2016/12/01 00:16:27, stanisc wrote: > > On 2016/11/30 23:49:05, danakj wrote: > > > How does the compositor be gone but the output surface is still alive? > > > > Honestly I don't know. Why was it WeakPtr<Compositor> in EstablishedGpuChannel > > the first place? > > I'll look into this. > > EstablishedGpuChannel is called from the GpuChannelFactory which is not tied to > the lifetime of a compositor in any way. Where as here the callback is from an > OutputSurface owned by the Display. The Display is owned by the > PerCompositorData which is destroyed in RemoveCompositor which is called from > ~Compositor. So it seems to be tied to the lifetime of the Compositor. OK, I've replaced this with a DCHECK. I think I still need this function to resolve WeakPtr. I am not comfortable passing Unretained() pointer to compositor in case the code that stores the callback changes later. https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/110001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:83: &BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu, On 2016/11/30 23:49:05, danakj wrote: > This is going to the base class just to call another callback. How about just > passing that callback here? Maybe the interval 0 code should move up to the GPTF > function? Done.
https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/browser_compositor_output_surface.h (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/browser_compositor_output_surface.h:42: void OnUpdateVSyncParametersFromGpu(base::TimeTicks timebase, This should go away? https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:81: if (vsync_provider) { nit: can remove {} https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:82: vsync_provider->GetVSyncParameters(update_vsync_parameters_callback_); It looks like this one has good reason to check the weakptr. "We provide the strong guarantee that the callback will not be called once the instance of this class is destroyed." And I don't see anything else cancelling the callback if this class is destroyed for instance?
https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/browser_compositor_output_surface.h (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/browser_compositor_output_surface.h:42: void OnUpdateVSyncParametersFromGpu(base::TimeTicks timebase, On 2016/12/02 21:44:14, danakj wrote: > This should go away? Right, will do. https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:82: vsync_provider->GetVSyncParameters(update_vsync_parameters_callback_); On 2016/12/02 21:44:14, danakj wrote: > It looks like this one has good reason to check the weakptr. "We provide the > strong guarantee that the callback will not be called once the instance of this > class is destroyed." And I don't see anything else cancelling the callback if > this class is destroyed for instance? I thought the comment "We provide the strong guarantee that the callback will not be called once the instance of this class is destroyed." is for the instance of VSyncProvider class. All existing implementation of VSyncProvider look like they invoke callback synchronously, but I agree the assumption about lifetime of the compositor vs. the callback cannot be guaranteed in this case. I am bit confused. I'd prefer to go back to check the WeakPtr<ui::Compositor> inside the callback implementation in GPTF. Another alternative is to make the callback function a member of ui::Compositor which as I understand would prevent it from being called if Compositor gets deleted. Would that be better?
https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:82: vsync_provider->GetVSyncParameters(update_vsync_parameters_callback_); On 2016/12/02 22:25:20, stanisc wrote: > On 2016/12/02 21:44:14, danakj wrote: > > It looks like this one has good reason to check the weakptr. "We provide the > > strong guarantee that the callback will not be called once the instance of > this > > class is destroyed." And I don't see anything else cancelling the callback if > > this class is destroyed for instance? > > I thought the comment "We provide the strong guarantee that the callback will > not be called once the instance of this class is destroyed." is for the instance > of VSyncProvider class. All existing implementation of VSyncProvider look like > they invoke callback synchronously, but I agree the assumption about lifetime of > the compositor vs. the callback cannot be guaranteed in this case. > > I am bit confused. I'd prefer to go back to check the WeakPtr<ui::Compositor> > inside the callback implementation in GPTF. Another alternative is to make the > callback function a member of ui::Compositor which as I understand would prevent > it from being called if Compositor gets deleted. Would that be better? OH I read this backwards, I thought it says it doesn't guarantee. But it does, and the VsyncProvider is owned by the SoftwareOutputDevice owned by this, so ya it's fine. 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...
https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/browser_compositor_output_surface.h (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/browser_compositor_output_surface.h:42: void OnUpdateVSyncParametersFromGpu(base::TimeTicks timebase, On 2016/12/02 22:25:20, stanisc wrote: > On 2016/12/02 21:44:14, danakj wrote: > > This should go away? > > Right, will do. Done. https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/software_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:81: if (vsync_provider) { On 2016/12/02 21:44:14, danakj wrote: > nit: can remove {} Done. https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/software_browser_compositor_output_surface.cc:82: vsync_provider->GetVSyncParameters(update_vsync_parameters_callback_); On 2016/12/02 22:35:10, danakj wrote: > On 2016/12/02 22:25:20, stanisc wrote: > > On 2016/12/02 21:44:14, danakj wrote: > > > It looks like this one has good reason to check the weakptr. "We provide the > > > strong guarantee that the callback will not be called once the instance of > > this > > > class is destroyed." And I don't see anything else cancelling the callback > if > > > this class is destroyed for instance? > > > > I thought the comment "We provide the strong guarantee that the callback will > > not be called once the instance of this class is destroyed." is for the > instance > > of VSyncProvider class. All existing implementation of VSyncProvider look like > > they invoke callback synchronously, but I agree the assumption about lifetime > of > > the compositor vs. the callback cannot be guaranteed in this case. > > > > I am bit confused. I'd prefer to go back to check the WeakPtr<ui::Compositor> > > inside the callback implementation in GPTF. Another alternative is to make the > > callback function a member of ui::Compositor which as I understand would > prevent > > it from being called if Compositor gets deleted. Would that be better? > > OH I read this backwards, I thought it says it doesn't guarantee. But it does, > and the VsyncProvider is owned by the SoftwareOutputDevice owned by this, so ya > it's fine. LGTM :) Thanks! One last change. I decided it would be better to eliminate the callback function from GPTF because it is now redundant and just point back to ui::CompositorSetDisplayVSyncParameters.
Description was changed from ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 ==========
LGTM https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/gpu_browser_compositor_output_surface.cc:39: GetCommandBufferProxy()->SetUpdateVSyncParametersCallback( I think this still has value. As it's the one who set up the connection, it should tear it down. https://codereview.chromium.org/2511273002/diff/150001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2511273002/diff/150001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:451: base::Bind(&ui::Compositor::SetDisplayVSyncParameters, compositor); That works too, cool.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)
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.
https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... File content/browser/compositor/gpu_browser_compositor_output_surface.cc (right): https://codereview.chromium.org/2511273002/diff/130001/content/browser/compos... content/browser/compositor/gpu_browser_compositor_output_surface.cc:39: GetCommandBufferProxy()->SetUpdateVSyncParametersCallback( On 2016/12/02 22:47:07, danakj wrote: > I think this still has value. As it's the one who set up the connection, it > should tear it down. Done.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2511273002/#ps170001 (title: "Bring back GpuBrowserCompositorOutputSurface destructor")
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": 170001, "attempt_start_ts": 1480726952862980,
"parent_rev": "23c519422fcb29527024deea2beadcd01451cd1f", "commit_rev":
"c326065399008990261787abd828bb219175f0bf"}
Message was sent while issue was closed.
Description was changed from ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 ========== to ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 ========== to ========== Decouple BrowserCompositorOutputSurface from BeginFrameSource. This change is a part of larger effort of propagating D3D VSync signal to the compositor. Since the current implementation in BrowserCompositorOutputSurface explicitly depends on a time based SyntheticBeginFrameSource, enne@ suggested that a good first step would be to try to decouple it from a specific BeginFrameSource type. Instead of passing SyntheticBeginFrameSource and CompositorVSyncManager in every constructor of BrowserCompositorOutputSurface and classes derived from it, this change replaces the SyntheticBFS / VSyncManager pair with a callback to update VSync parameters. The callback is handled at GpuProcessTransportFactory. BUG=467617 Committed: https://crrev.com/3566f880e80d65a57b7e084ccd55030972585c98 Cr-Commit-Position: refs/heads/master@{#436122} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3566f880e80d65a57b7e084ccd55030972585c98 Cr-Commit-Position: refs/heads/master@{#436122} |
