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

Issue 658607 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 665683



Sign in to add a comment

Eliminate SurfaceFactory::Create, and SurfaceFactory::Destroy

Project Member Reported by fsam...@chromium.org, Oct 23 2016

Issue description

This bug is based on a couple of observations.

1. SurfaceFactory(Client) and CompositorFrameSink(Client) are converging and should become one thing in the future to dedup a lot of code (hundreds if not thousands of lines of code).

2. All CompositorFrameSink implementations use SurfaceFactory in essentially the same way: if we need a new surface ID, then we create it and destroy the old one.

In an effort to minimize SurfaceFactory's API footprint and continue to converge SurfaceFactory and CompositorFrameSink, I propose deleting SurfaceFactory::Create, SurfaceFactory::Destroy, and adding a LocalFrameId to the SubmitCompositorFrame interface on SurfaceFactory:

void SurfaceFactory::SubmitCompositorFrame(const LocalFrameId& local_frame_id, CompositorFrame compositor_frame);

Internally, if the local_frame_id changes, then we throw out the old Surface, and create a new one.
 

Comment 1 by danakj@chromium.org, Oct 24 2016

Is it weird that we'd be passing an Id to Display::SetSurfaceId before it got to the factory then? https://cs.chromium.org/chromium/src/cc/surfaces/direct_compositor_frame_sink.cc?rcl=1477308281&l=98
It is weird. Does it have to happen in that order? Can SubmitCompositorFrame happen before SetSurfaceId?

Comment 3 by danakj@chromium.org, Oct 24 2016

I dunno what the side effects can be, we'd have to look at the methods and justify it.
Owner: samans@chromium.org
Status: Assigned (was: Untriaged)
+samans@

I have to say, I don't understand the reasoning for putting SubmitCompositorFrame AFTER SetSurfaceId upon a superficial skim of the code. In fact, here we're marking a Surface as being damaged before it has been submitted:

https://cs.chromium.org/chromium/src/cc/surfaces/display_scheduler.cc?rcl=1477451763&l=85

I don't think this matters in practice because nothing else happens on the same call stack I think. We schedule a BeginFrame but that won't happen on the same call stack.

I think flipping the order is fine.

Also, revisiting SurfaceFactory, we already have LocalFrameId as the first parameter. :P I was not reading the code as I wrote this bug it seems :P

So yea, I think we can get rid of Create and Destroy and make them internal operations within SubmitCompositorFrame.

SurfaceFactory::SubmitCompositorFrame has a callback today but we can just as well make it SurfaceFactoryClient::DidReceiveCompositorFrameAck to progress us towards unifying CompositorFrameSink and SurfaceFactory.

Comment 5 by danakj@chromium.org, Oct 27 2016

> SurfaceFactory::SubmitCompositorFrame has a callback today but we can just as well make it SurfaceFactoryClient::DidReceiveCompositorFrameAck to progress us towards unifying CompositorFrameSink and SurfaceFactory.

Would that make things both being a SurfaceFactoryClient and having a SurfaceFactoryClient? Leaving the callback might be nicer than that until we resolve that.
Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 16 2016

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

commit b902fd366ba91757dbee99d5f821f00d2de1d181
Author: samans <samans@chromium.org>
Date: Wed Nov 16 00:25:06 2016

Remove SurfaceFactory::Create and SurfaceFactory::Destroy

It is no longer possible to explicitly construct and destroy surfaces. A
SurfaceFactory instance now handles only one surface at a time. Once the
local frame id passed to SubmitCompositorFrame changes, the factory gets
rid of the old surface and creates a new one.

BUG= 658607 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2485473003
Cr-Commit-Position: refs/heads/master@{#432312}

[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/blimp/client/core/compositor/blimp_compositor.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/display_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_aggregator_perftest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_factory.h
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_manager_ref_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surface_unittest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/surfaces/surfaces_pixeltest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/components/exo/surface.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/content/renderer/android/synchronous_compositor_frame_sink.h
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/services/ui/ws/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181/ui/android/delegated_frame_host_android.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 16 2016

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

commit 5cdae2b53317c08448d0e765a0443934deb788c3
Author: suzyh <suzyh@chromium.org>
Date: Wed Nov 16 03:07:30 2016

Revert of Remove SurfaceFactory::Create and SurfaceFactory::Destroy (patchset #40 id:880001 of https://codereview.chromium.org/2485473003/ )

Reason for revert:
Seems to have broken windows compile:
https://build.chromium.org/p/chromium/builders/Win/builds/49156

Original issue's description:
> Remove SurfaceFactory::Create and SurfaceFactory::Destroy
>
> It is no longer possible to explicitly construct and destroy surfaces. A
> SurfaceFactory instance now handles only one surface at a time. Once the
> local frame id passed to SubmitCompositorFrame changes, the factory gets
> rid of the old surface and creates a new one.
>
> BUG= 658607 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
>
> Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181
> Cr-Commit-Position: refs/heads/master@{#432312}

TBR=reveman@chromium.org,boliu@chromium.org,danakj@chromium.org,dtrainor@chromium.org,fsamuel@chromium.org,jbauman@chromium.org,piman@chromium.org,sky@chromium.org,samans@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 658607 

Review-Url: https://codereview.chromium.org/2506883002
Cr-Commit-Position: refs/heads/master@{#432370}

[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/blimp/client/core/compositor/blimp_compositor.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/display_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_aggregator_perftest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_factory.h
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_manager_ref_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surface_unittest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/surfaces/surfaces_pixeltest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/components/exo/surface.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/content/renderer/android/synchronous_compositor_frame_sink.h
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/services/ui/ws/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/5cdae2b53317c08448d0e765a0443934deb788c3/ui/android/delegated_frame_host_android.cc

Comment 9 by kbr@chromium.org, Nov 16 2016

Blockedon: 665683
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 28 2016

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

commit 46b4f407905420c8997e5c886baef4bbf5248b79
Author: samans <samans@chromium.org>
Date: Mon Nov 28 21:47:13 2016

Remove SurfaceFactory::Create and SurfaceFactory::Destroy

It is no longer possible to explicitly construct and destroy surfaces. A
SurfaceFactory instance now handles only one surface at a time. Once the
local frame id passed to SubmitCompositorFrame changes, the factory gets
rid of the old surface and creates a new one.

BUG= 658607 

Committed: https://crrev.com/b902fd366ba91757dbee99d5f821f00d2de1d181
Cr-Commit-Position: refs/heads/master@{#432312}
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2485473003
Cr-Commit-Position: refs/heads/master@{#434724}

[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/android_webview/browser/hardware_renderer.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/android_webview/browser/surfaces_instance.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/blimp/client/core/compositor/blimp_compositor.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/direct_compositor_frame_sink.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/display_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_aggregator_perftest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_aggregator_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_factory.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_factory.h
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_factory_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_hittest_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_manager_ref_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surface_unittest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/surfaces/surfaces_pixeltest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/cc/test/test_compositor_frame_sink.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/components/exo/surface.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/browser/frame_host/render_widget_host_view_child_frame.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/browser/frame_host/render_widget_host_view_guest.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/browser/renderer_host/delegated_frame_host.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/renderer/android/synchronous_compositor_frame_sink.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/content/renderer/android/synchronous_compositor_frame_sink.h
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/services/ui/surfaces/gpu_compositor_frame_sink.cc
[modify] https://crrev.com/46b4f407905420c8997e5c886baef4bbf5248b79/ui/android/delegated_frame_host_android.cc

Status: Fixed (was: Started)

Sign in to add a comment