Consolidate OutputSurface constructors into GL vs Vulkan.

Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}

Committed: https://crrev.com/d7b005182e7a5174db8aa8e763df67faa46c4cd5
Cr-Commit-Position: refs/heads/master@{#395728}

[danakj, 2016-05-23 21:09:54]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
BUG=487471
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-05-23 21:12:06]

danakj@chromium.org changed reviewers:
+ nyquist@chromium.org, sky@chromium.org

[danakj, 2016-05-23 21:12:07]

+sky for components/mus/
+nyquist for blimp/

[danakj, 2016-05-23 21:12:36]

danakj@chromium.org changed reviewers:
+ torne@chromium.org

[danakj, 2016-05-23 21:12:37]

+torne since bo is out.

[danakj, 2016-05-23 21:14:28]

https://codereview.chromium.org/2002303002/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:50:
capabilities_.delegated_rendering = true;
Oh. Mailbox output surface :/ I can't do this.

[nyquist, 2016-05-23 21:21:16]

blimp lgtm

[rjkroege, 2016-05-23 21:30:06]

rjkroege@chromium.org changed reviewers:
+ rjkroege@chromium.org

[rjkroege, 2016-05-23 21:30:07]

https://codereview.chromium.org/2002303002/diff/20001/components/mus/surfaces...
File components/mus/surfaces/direct_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/20001/components/mus/surfaces...
components/mus/surfaces/direct_output_surface.cc:21: :
cc::OutputSurface(context_provider, nullptr, nullptr),
Perhaps DirectOutputSurfaceOzone's invocation of cc::OutputSurface needs to be
updated too?

[danakj, 2016-05-23 21:30:46]

https://codereview.chromium.org/2002303002/diff/20001/components/mus/surfaces...
File components/mus/surfaces/direct_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/20001/components/mus/surfaces...
components/mus/surfaces/direct_output_surface.cc:21: :
cc::OutputSurface(context_provider, nullptr, nullptr),
On 2016/05/23 21:30:07, rjkroege wrote:
> Perhaps DirectOutputSurfaceOzone's invocation of cc::OutputSurface needs to be
> updated too?

Ya probably, I didn't try to build ozone locally.

[danakj, 2016-05-23 21:31:26]

https://codereview.chromium.org/2002303002/diff/20001/content/renderer/gpu/co...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/20001/content/renderer/gpu/co...
content/renderer/gpu/compositor_output_surface.cc:50:
capabilities_.delegated_rendering = true;
On 2016/05/23 21:14:28, danakj wrote:
> Oh. Mailbox output surface :/ I can't do this.

I'm going to try split Mailbox from Compositor output surface, removing layout
test code from CompositorOutputSurface.

[danakj, 2016-05-23 21:48:04]

ok MailboxOutputSurface no longer subclasses CompositorOutputSurface, and the
latter no longer knows about layout tests.

[piman, 2016-05-23 22:23:51]

LGTM, with a couple of suggested improvements.

https://codereview.chromium.org/2002303002/diff/40001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2002303002/diff/40001/cc/output/output_surfac...
cc/output/output_surface.h:54: // Constructor for Vulkan-based and/or software
compositing.
nit: For software compositing, we would use the above constructor, so I don't
think it's useful to leave the software_device in this one.

https://codereview.chromium.org/2002303002/diff/40001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/40001/cc/test/fake_output_sur...
cc/test/fake_output_surface.cc:44: FakeOutputSurface::FakeOutputSurface(
nit, if you want, you can trivially delegate the other 2 constructors to this
one.

[danakj, 2016-05-23 22:28:55]

https://codereview.chromium.org/2002303002/diff/40001/cc/output/output_surface.h
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2002303002/diff/40001/cc/output/output_surfac...
cc/output/output_surface.h:54: // Constructor for Vulkan-based and/or software
compositing.
On 2016/05/23 22:23:51, piman wrote:
> nit: For software compositing, we would use the above constructor, so I don't
> think it's useful to leave the software_device in this one.

Ya, kinda, until Webview uses vulkan and then we need to have both. But I
suppose we can bet on splitting OutputSurface vs LTHI before Webview gets to
that.. :)

https://codereview.chromium.org/2002303002/diff/40001/cc/test/fake_output_sur...
File cc/test/fake_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/40001/cc/test/fake_output_sur...
cc/test/fake_output_surface.cc:44: FakeOutputSurface::FakeOutputSurface(
On 2016/05/23 22:23:51, piman wrote:
> nit, if you want, you can trivially delegate the other 2 constructors to this
> one.

Good plan.

[danakj, 2016-05-23 22:55:23]

Bunch of new changes in components/display_compositor/ re buffer queue (and
unittest). I made it take a GLES2Interface* instead of a
scoped_refptr<ContextProvider> which helps separate lifetime concerns.

Also updated DirectOutputSurfaceOzone and
SurfacelessGpuBrowserCompositorOutputSurface.

[danakj, 2016-05-23 22:57:11]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 22:57:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002303002/60001

[danakj, 2016-05-23 23:04:19]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 23:05:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002303002/120001

[danakj, 2016-05-23 23:05:29]

I reduced the diff in the DirectOSOzone class, I was moving use of the context
provider to after BindToClient, but this is for Display compositing which is
going to drop the concept of BindToClient probably, or at least the concept of
using that to create a context/bind it to thread.

[piman, 2016-05-23 23:12:39]

lgtm

https://codereview.chromium.org/2002303002/diff/120001/cc/output/output_surfa...
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2002303002/diff/120001/cc/output/output_surfa...
cc/output/output_surface.h:50: explicit
OutputSurface(scoped_refptr<ContextProvider> context_provider,
nit: no need for explicit?

[danakj, 2016-05-23 23:16:01]

https://codereview.chromium.org/2002303002/diff/120001/cc/output/output_surfa...
File cc/output/output_surface.h (right):

https://codereview.chromium.org/2002303002/diff/120001/cc/output/output_surfa...
cc/output/output_surface.h:50: explicit
OutputSurface(scoped_refptr<ContextProvider> context_provider,
On 2016/05/23 23:12:38, piman wrote:
> nit: no need for explicit?

Kinda but actually I think you can hit multi-param constructors implicitly now,
due to {} initialization. explicit on everything seems good.

[danakj, 2016-05-23 23:21:01]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-05-23 23:25:12]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-05-23 23:25:13]

lgtm

[danakj, 2016-05-23 23:25:57]

I'll TBR torne and sky for what's left

[danakj, 2016-05-23 23:26:07]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-05-23 23:26:13]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-05-23 23:26:14]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/2002303002/#ps120001
(title: "outputsurface-constructors: ozonesmallerdiff")

[commit-bot: I haz the power, 2016-05-23 23:26:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002303002/120001

[commit-bot: I haz the power, 2016-05-24 00:20:39]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-24 00:20:41]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-24 00:21:56]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

[commit-bot: I haz the power, 2016-05-24 00:21:58]

Patchset 7 (id:??) landed as
https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}

[tkent, 2016-05-24 05:33:45]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2009443002/ by tkent@chromium.org.

The reason for reverting is: Caused 14,366 crashes on Windows layout test bots.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/42641
.

[danakj, 2016-05-24 20:46:53]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

[danakj, 2016-05-24 20:54:32]

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_output_surface.cc (left):

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:125: if (context_provider()) {
Hm this if (context_provider()) notably was removed in this patch.

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/m...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/m...
content/renderer/gpu/mailbox_output_surface.cc:243: gpu::gles2::GLES2Interface*
gl = context_provider_->ContextGL();
But youd think if it was null, then calling InsertFenceSyncCHROMIUM() on the
ContextGL() would crash first. Or even GetGraphcisResetStatusKHR.

[danakj, 2016-05-24 20:58:10]

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/m...
File content/renderer/gpu/mailbox_output_surface.cc (right):

https://codereview.chromium.org/2002303002/diff/120001/content/renderer/gpu/m...
content/renderer/gpu/mailbox_output_surface.cc:254:
base::Passed(&frame->gl_frame_data)));
Ohh we're passing gl_frame_data here, but using it for the sync_token as a
parameter also. I'm sure that's it, we end up with a null gl_frame_data.

[danakj, 2016-05-24 21:01:27]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-05-24 21:01:36]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-05-24 21:01:52]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

[danakj, 2016-05-24 21:02:00]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 21:02:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002303002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002303002/140001

[danakj, 2016-05-24 21:06:56]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-05-24 21:07:01]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-05-24 21:07:01]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
nyquist@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2002303002/#ps140001
(title: "outputsurface-constructors: rebase-and-fixcrash")

[danakj, 2016-05-24 21:07:19]

I added win_blink_rel to the CQ bots for this trip.

[commit-bot: I haz the power, 2016-05-24 21:07:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002303002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002303002/140001

[commit-bot: I haz the power, 2016-05-24 23:22:01]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

[commit-bot: I haz the power, 2016-05-24 23:22:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-24 23:24:22]

Description was changed from

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}
==========

to

==========
Consolidate OutputSurface constructors into GL vs Vulkan.

Get rid of the many many constructors on OutputSurface, drop it down
to constructors for GL based compositing vs Vulkan based.

R=boliu@chromium.org, piman, sievers
TBR=sky, torne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.blink:win_blink_rel

Committed: https://crrev.com/6daedd173180936d9c56976a276a91ff761da68a
Cr-Commit-Position: refs/heads/master@{#395478}

Committed: https://crrev.com/d7b005182e7a5174db8aa8e763df67faa46c4cd5
Cr-Commit-Position: refs/heads/master@{#395728}
==========

[commit-bot: I haz the power, 2016-05-24 23:24:25]

Patchset 8 (id:??) landed as
https://crrev.com/d7b005182e7a5174db8aa8e763df67faa46c4cd5
Cr-Commit-Position: refs/heads/master@{#395728}

[hajimehoshi, 2016-05-25 08:36:21]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2013743002/ by hajimehoshi@chromium.org.

The reason for reverting is: This causes flaky test failures (e.g.
https://build.chromium.org/p/chromium.gpu/builders/Linux%20Release%20%28NVIDI...).

[boliu, 2016-05-31 18:59:12]

lgtm fwiw