Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683583005:
Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink (Closed)
DescriptionMove display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink
With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate
interface, CompositorFrameSinkSupport doesn't need to know about cc::Display.
cc::Display::SetLocalSurfaceId() checks and early returns if
neither local surface id nor device scale factor has changed.
Therefore, FrameGenerator's calling SetLocalSurfaceId() before
every SubmitCompositorFrame() doesn't result in extra work.
This CL also fixes bug 689869. The bug was caused by
https://codereview.chromium.org/2612083002/.
In the previous CL, cc::Display::Initialize() was called inside
CompositorFrameSinkSupport(), where CompositorFrameSinkSupport
was passed as DisplayClient.
CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't
doing anything so the CompositorFrameSinkClient didn't get
notified when we lost CompositorFrameSink.
For bug 676070, 689916 (flaky tests):
The two bugs are caused by CompositorFrameSinkSupport doesn't stop
observing BeginFrameSource when it should. By notifying client on output
surface lost correctly, DirectCompositorFrameSink detaches from client
and destroys CompositorFrameSinkSupport (and hence stop observing
BeginFrameSource) at the right time.
BUG=688042, 689869, 676070, 689916
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2683583005
Cr-Commit-Position: refs/heads/master@{#449777}
Committed: https://chromium.googlesource.com/chromium/src/+/04c49bf4845979aeff2c740a168de0a936b015af
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : addressed comments #
Total comments: 20
Patch Set 4 : Addressed comments #Patch Set 5 : addressed comments #
Total comments: 4
Patch Set 6 : addressed comments #
Total comments: 12
Patch Set 7 : Added TODO to DisplayOutputSurfaceLost #Patch Set 8 : Add DCHECK(display_) in DirectCompositorFrameSink #Patch Set 9 : addressed comments #Patch Set 10 : rebase #Messages
Total messages: 79 (51 generated)
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. BUG=688042 ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. BUG=688042 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by staraz@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_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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 staraz@chromium.org to run a CQ dry run
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. BUG=688042 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
staraz@chromium.org changed reviewers: + danakj@chromium.org, fsamuel@chromium.org, xidachen@chromium.org
xidachen@chromium.org: Please review changes in offscreen canvas danakj@chromium.org: Please review changes in cc/surfaces and components/display_compositor
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by staraz@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...
Patchset #2 (id:20001) has been deleted
I think you need to update exo too? https://cs.chromium.org/chromium/src/components/exo/compositor_frame_sink.cc?... Otherwise, this generally LGTM https://codereview.chromium.org/2683583005/diff/40001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/40001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} New line between each method.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683583005/diff/40001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/40001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} On 2017/02/08 17:54:42, Fady Samuel wrote: > New line between each method. Done.
staraz@chromium.org changed reviewers: + reveman@chromium.org
reveman@chromium.org: Please review changes in components/exo
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Could you please also add in this description what bug this fixes? Thanks!
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869, which is caused by client not being notified by DisplayOutputSurfaceLost(). BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/02/08 18:53:08, Fady Samuel wrote: > Could you please also add in this description what bug this fixes? Thanks! Done.
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869, which is caused by client not being notified by DisplayOutputSurfaceLost(). BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the CL, cc::Display::Initialize() is called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the CL, cc::Display::Initialize() is called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() is called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() is called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:27: bool has_display) it seems a bit unmeaningful to call this has_display for this class, since it doesn't use or know about a display. what does has_display mean? that it's the root? something else? can you describe that in the name and maybe comments on the defn? https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:72: capabilities_.delegated_sync_points_required, true /* has_display */); nit: here you could use a temp bool var to give the |true| a name, and that's generally nicer than a comment when possible IMO https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_compositor_frame_sink.cc:22: surface_manager, Pointing out that this class has a lot of constructor params that are only there to forward to the support, and it should probably take a support as an argument instead. https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:39: void GpuDisplayCompositorFrameSink::SubmitCompositorFrame( Overloading a method that has an impl in the parent class feels kinda bad and makes me wonder if we have the right hierarchy? Should this merge with its parent? Something else? https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} Why does the client not want to hear about the context being lost?
xidachen@chromium.org changed reviewers: + xlai@chromium.org - xidachen@chromium.org
xidachen@chromium.org changed reviewers: + xidachen@chromium.org
-xidachen@, +xlai@ xlai@: please review offscreen canvas changes.
https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:119: // If this is a display root surface and the SurfaceId is changing, make the Not sure if this comment about "display root surface" is still relevant after the code change. Same question for other similar places in this class. https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:77: // Avoid initializing GL context here, as this should be sharing the Display::Initialize may initialize the GLRenderer so I'm wondering if this comment is still relevant to the new code change. https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:79: display_->Initialize(this, surface_manager_); DCHECK(display_) ? https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:97: display_->SetLocalSurfaceId(delegated_local_surface_id_, DCHECK(display_) ? https://codereview.chromium.org/2683583005/diff/60001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/exo/surface.... components/exo/surface.cc:23: #include "cc/surfaces/surface_manager.h" Why include this?
The CQ bit was checked by staraz@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 checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... File cc/surfaces/compositor_frame_sink_support.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:27: bool has_display) On 2017/02/08 19:23:38, danakj (slow) wrote: > it seems a bit unmeaningful to call this has_display for this class, since it > doesn't use or know about a display. what does has_display mean? that it's the > root? something else? can you describe that in the name and maybe comments on > the defn? Done. https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/compositor_... cc/surfaces/compositor_frame_sink_support.cc:119: // If this is a display root surface and the SurfaceId is changing, make the On 2017/02/08 20:11:34, xlai (Olivia) wrote: > Not sure if this comment about "display root surface" is still relevant after > the code change. Same question for other similar places in this class. Yes, it's still relevant. Checking display_ is not null is equivalent to checking submits_to_display_compositor_ (has_display_) to be true. https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:72: capabilities_.delegated_sync_points_required, true /* has_display */); On 2017/02/08 19:23:38, danakj (slow) wrote: > nit: here you could use a temp bool var to give the |true| a name, and that's > generally nicer than a comment when possible IMO Done. https://codereview.chromium.org/2683583005/diff/60001/cc/surfaces/direct_comp... cc/surfaces/direct_compositor_frame_sink.cc:77: // Avoid initializing GL context here, as this should be sharing the On 2017/02/08 20:11:34, xlai (Olivia) wrote: > Display::Initialize may initialize the GLRenderer so I'm wondering if this > comment is still relevant to the new code change. It is. Most changes in this file are to restore the changes made in this CL: https://codereview.chromium.org/2612083002/. That's also why there's no DCHECKs for display_. https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_compositor_frame_sink.cc:22: surface_manager, On 2017/02/08 19:23:38, danakj (slow) wrote: > Pointing out that this class has a lot of constructor params that are only there > to forward to the support, and it should probably take a support as an argument > instead. Done. https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:39: void GpuDisplayCompositorFrameSink::SubmitCompositorFrame( On 2017/02/08 19:23:38, danakj (slow) wrote: > Overloading a method that has an impl in the parent class feels kinda bad and > makes me wonder if we have the right hierarchy? Should this merge with its > parent? Something else? As discussed on slack, DisplayPrivate::SetLocalSurfaceId() is added. FrameGenerator calls SetLocalSurfaceId() before submitting a CompositorFrame to avoid this override. https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} On 2017/02/08 19:23:38, danakj (slow) wrote: > Why does the client not want to hear about the context being lost? The client_ (assuming we are changing it from private to protected in GpuCompositorFrameSink) is a MojoCompositorFrameSinkClient. It doesn't have anything to call for context lost. I can add something like that on a separate CL if it's needed. Besides, these DisplayClient implementations are moved from CompositorFrameSinkSupport, which wasn't doing anything in the first place. https://codereview.chromium.org/2683583005/diff/60001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/exo/surface.... components/exo/surface.cc:23: #include "cc/surfaces/surface_manager.h" On 2017/02/08 20:11:34, xlai (Olivia) wrote: > Why include this? SurfaceManager::reference_factory() is called at line 180. Several bots were complaining of not having a complete SurfaceManager type when running with patchset 2.
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/2683583005/diff/100001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:120: // |has_display_| is true if the CompositorFrameSinkSupport submits This comment is stale. https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:70: const bool submits_to_display_compositor = true; constexpr
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:120: // |has_display_| is true if the CompositorFrameSinkSupport submits On 2017/02/08 21:37:46, Fady Samuel wrote: > This comment is stale. Done. https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/100001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:70: const bool submits_to_display_compositor = true; On 2017/02/08 21:37:46, Fady Samuel wrote: > constexpr Done.
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/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} On 2017/02/08 21:26:47, StarAZ1 wrote: > On 2017/02/08 19:23:38, danakj (slow) wrote: > > Why does the client not want to hear about the context being lost? > > The client_ (assuming we are changing it from private to protected in > GpuCompositorFrameSink) is a MojoCompositorFrameSinkClient. It > doesn't have anything to call for context lost. I can add something > like that on a separate CL if it's needed. Besides, these > DisplayClient implementations are moved from > CompositorFrameSinkSupport, which wasn't doing anything in the > first place. If the display's context/outputsurface is lost, then the client will have to resubmit a frame with all its resources (and probably recreate them cuz its own context is lost). This isn't a layer-tree specific part of the cc::CompositorFrameSinkClient. https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink_client.h... Indeed it wasn't doing anything which was a bug you saw for DisplayCompositor but other submitters of frames will want to know also. Let's make sure we follow up. Perhaps the MojoCompositorFrameSinkClient just needs to lose its mojo connection at that point or something, rather than adding another method - not sure about that part. But the MojoCompositorFrameSink is dead if its output surface/context is lost.
staraz@chromium.org changed reviewers: + msw@chromium.org, tsepez@chromium.org
msw@chromium.org: Please review changes in frame_generator.cc tsepez@: Please review display_compositor.mojom
xlai@chromium.org changed reviewers: - xidachen@chromium.org
lgtm with nits https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:80: // Display's context. Add DCHECK(display_);
https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... File components/display_compositor/gpu_display_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/60001/components/display_comp... components/display_compositor/gpu_display_compositor_frame_sink.cc:70: void GpuDisplayCompositorFrameSink::DisplayOutputSurfaceLost() {} On 2017/02/08 21:42:43, danakj (slow) wrote: > On 2017/02/08 21:26:47, StarAZ1 wrote: > > On 2017/02/08 19:23:38, danakj (slow) wrote: > > > Why does the client not want to hear about the context being lost? > > > > The client_ (assuming we are changing it from private to protected in > > GpuCompositorFrameSink) is a MojoCompositorFrameSinkClient. It > > doesn't have anything to call for context lost. I can add something > > like that on a separate CL if it's needed. Besides, these > > DisplayClient implementations are moved from > > CompositorFrameSinkSupport, which wasn't doing anything in the > > first place. > > If the display's context/outputsurface is lost, then the client will have to > resubmit a frame with all its resources (and probably recreate them cuz its own > context is lost). This isn't a layer-tree specific part of the > cc::CompositorFrameSinkClient. > https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink_client.h... > > Indeed it wasn't doing anything which was a bug you saw for DisplayCompositor > but other submitters of frames will want to know also. Let's make sure we follow > up. Perhaps the MojoCompositorFrameSinkClient just needs to lose its mojo > connection at that point or something, rather than adding another method - not > sure about that part. But the MojoCompositorFrameSink is dead if its output > surface/context is lost. I added a TODO here.
https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:123: // texture/bitmap. (e.g. OffscreenCanvasCompositorFrameSink and These class names may become stale and I think the description is good enough without them. https://codereview.chromium.org/2683583005/diff/120001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:7: #include "cc/surfaces/surface_manager.h" is needed? this doesn't even take a surfacemanager* anymore https://codereview.chromium.org/2683583005/diff/120001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/components/exo/composi... components/exo/compositor_frame_sink.cc:26: false /* has_display */), update this https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:92: display_private_->SetLocalSurfaceId(local_surface_id_, DirectCompositorFrameSink::SubmitCompositorFrame doesn't do this only when the size changes. Why the inconsistency?
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. BUG=688042, 689869 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. For bug 676070, 689916 (flaky tests): The two bugs are caused by CompositorFrameSinkSupport doesn't stop observing BeginFrameSource when it should. By notifying client on output surface lost correctly, DirectCompositorFrameSink detaches from client and destroys CompositorFrameSinkSupport (and hence stop observing BeginFrameSource) at the right time. BUG=688042, 689869, 676070, 689916 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/direct_com... File cc/surfaces/direct_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/direct_com... cc/surfaces/direct_compositor_frame_sink.cc:80: // Display's context. On 2017/02/08 21:48:35, xlai (Olivia) wrote: > Add DCHECK(display_); Done.
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/2683583005/diff/120001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support.h (right): https://codereview.chromium.org/2683583005/diff/120001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support.h:123: // texture/bitmap. (e.g. OffscreenCanvasCompositorFrameSink and On 2017/02/08 21:54:40, danakj (slow) wrote: > These class names may become stale and I think the description is good enough > without them. Done. https://codereview.chromium.org/2683583005/diff/120001/components/display_com... File components/display_compositor/gpu_compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/components/display_com... components/display_compositor/gpu_compositor_frame_sink.cc:7: #include "cc/surfaces/surface_manager.h" On 2017/02/08 21:54:40, danakj (slow) wrote: > is needed? this doesn't even take a surfacemanager* anymore Done. https://codereview.chromium.org/2683583005/diff/120001/components/exo/composi... File components/exo/compositor_frame_sink.cc (right): https://codereview.chromium.org/2683583005/diff/120001/components/exo/composi... components/exo/compositor_frame_sink.cc:26: false /* has_display */), On 2017/02/08 21:54:40, danakj (slow) wrote: > update this Done. https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:92: display_private_->SetLocalSurfaceId(local_surface_id_, On 2017/02/08 21:54:40, danakj (slow) wrote: > DirectCompositorFrameSink::SubmitCompositorFrame doesn't do this only when the > size changes. Why the inconsistency? I wanted to only do this when local surface id changes. I'll move it out to be consistent with DirectCompositorFrameSink.
frame_generator.cc rubber stamp lgtm
The CQ bit was checked by staraz@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...
Patchset #10 (id:190001) has been deleted
https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:92: display_private_->SetLocalSurfaceId(local_surface_id_, On 2017/02/08 22:07:52, StarAZ1 wrote: > On 2017/02/08 21:54:40, danakj (slow) wrote: > > DirectCompositorFrameSink::SubmitCompositorFrame doesn't do this only when the > > size changes. Why the inconsistency? > > I wanted to only do this when local surface id changes. > I'll move it out to be consistent with DirectCompositorFrameSink. Well.. we shouldn't do work we don't need to but I want to understand what is different here. Why does DirectCompositorFrameSink need to do this when resize didn't happen but this doesn't? We can document this with comments.
mojom lgtm
The CQ bit was checked by staraz@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...
Patchset #10 (id:210001) has been deleted
The CQ bit was checked by staraz@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/2683583005/diff/120001/services/ui/ws/frame_g... File services/ui/ws/frame_generator.cc (right): https://codereview.chromium.org/2683583005/diff/120001/services/ui/ws/frame_g... services/ui/ws/frame_generator.cc:92: display_private_->SetLocalSurfaceId(local_surface_id_, On 2017/02/08 23:31:16, danakj wrote: > On 2017/02/08 22:07:52, StarAZ1 wrote: > > On 2017/02/08 21:54:40, danakj (slow) wrote: > > > DirectCompositorFrameSink::SubmitCompositorFrame doesn't do this only when > the > > > size changes. Why the inconsistency? > > > > I wanted to only do this when local surface id changes. > > I'll move it out to be consistent with DirectCompositorFrameSink. > > Well.. we shouldn't do work we don't need to but I want to understand what is > different here. Why does DirectCompositorFrameSink need to do this when resize > didn't happen but this doesn't? We can document this with comments. Display::SetLocalSurfaceId() checks at the beginning and early returns if neither local_surface_id_ nor device_scale_factor_ has changed.
LGTM
lgtm
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. This CL restores DirectCompositorFrameSink to be the DisplayClient. DirectCompositorFrameSink::DisplayOutputSurfaceLost() calls client_->DidLoseCompositorFrameSink() to notify its client about the lost CompositorFrameSinkClient. For bug 676070, 689916 (flaky tests): The two bugs are caused by CompositorFrameSinkSupport doesn't stop observing BeginFrameSource when it should. By notifying client on output surface lost correctly, DirectCompositorFrameSink detaches from client and destroys CompositorFrameSinkSupport (and hence stop observing BeginFrameSource) at the right time. BUG=688042, 689869, 676070, 689916 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. cc::Display::SetLocalSurfaceId() checks and early returns if neither local surface id nor device scale factor has changed. Therefore, FrameGenerator's calling SetLocalSurfaceId() before every SubmitCompositorFrame() doesn't result in extra work. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. For bug 676070, 689916 (flaky tests): The two bugs are caused by CompositorFrameSinkSupport doesn't stop observing BeginFrameSource when it should. By notifying client on output surface lost correctly, DirectCompositorFrameSink detaches from client and destroys CompositorFrameSinkSupport (and hence stop observing BeginFrameSource) at the right time. BUG=688042, 689869, 676070, 689916 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the explanation, LGTM
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xlai@chromium.org, msw@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2683583005/#ps230001 (title: "rebase")
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": 230001, "attempt_start_ts": 1486767929934990,
"parent_rev": "6d03fad48660f6a6a767c6cc70c621ff10f8fcfa", "commit_rev":
"04c49bf4845979aeff2c740a168de0a936b015af"}
Message was sent while issue was closed.
Description was changed from ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. cc::Display::SetLocalSurfaceId() checks and early returns if neither local surface id nor device scale factor has changed. Therefore, FrameGenerator's calling SetLocalSurfaceId() before every SubmitCompositorFrame() doesn't result in extra work. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. For bug 676070, 689916 (flaky tests): The two bugs are caused by CompositorFrameSinkSupport doesn't stop observing BeginFrameSource when it should. By notifying client on output surface lost correctly, DirectCompositorFrameSink detaches from client and destroys CompositorFrameSinkSupport (and hence stop observing BeginFrameSource) at the right time. BUG=688042, 689869, 676070, 689916 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Move display_ From CompositorFrameSinkSupport To GpuDisplayCompositorFrameSink With FrameGenerator and GpuDisplayCompositorFrameSink having a DisplayPrivate interface, CompositorFrameSinkSupport doesn't need to know about cc::Display. cc::Display::SetLocalSurfaceId() checks and early returns if neither local surface id nor device scale factor has changed. Therefore, FrameGenerator's calling SetLocalSurfaceId() before every SubmitCompositorFrame() doesn't result in extra work. This CL also fixes bug 689869. The bug was caused by https://codereview.chromium.org/2612083002/. In the previous CL, cc::Display::Initialize() was called inside CompositorFrameSinkSupport(), where CompositorFrameSinkSupport was passed as DisplayClient. CompositorFrameSinkSupport::DisplayOutputSurfaceLost() wasn't doing anything so the CompositorFrameSinkClient didn't get notified when we lost CompositorFrameSink. For bug 676070, 689916 (flaky tests): The two bugs are caused by CompositorFrameSinkSupport doesn't stop observing BeginFrameSource when it should. By notifying client on output surface lost correctly, DirectCompositorFrameSink detaches from client and destroys CompositorFrameSinkSupport (and hence stop observing BeginFrameSource) at the right time. BUG=688042, 689869, 676070, 689916 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2683583005 Cr-Commit-Position: refs/heads/master@{#449777} Committed: https://chromium.googlesource.com/chromium/src/+/04c49bf4845979aeff2c740a168d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:230001) as https://chromium.googlesource.com/chromium/src/+/04c49bf4845979aeff2c740a168d... |
