Refactor GL surface format handling

Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002 which is now forked into https://crrev.com/2586803003 .)

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2616723002
Cr-Commit-Position: refs/heads/master@{#441993}
Committed: https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbcfd740af453c7

[klausw, 2017-01-04 21:55:59]

Description was changed from

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

The old enums are still used as shortcuts for initializing the
GLSurfaceFormat objects, these could be replaced with more
specific builder-stule initialization in the future if needed.

BUG=
==========

to

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

The old enums are still used as shortcuts for initializing the
GLSurfaceFormat objects, these could be replaced with more
specific builder-stule initialization in the future if needed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[klausw, 2017-01-04 21:57:11]

klausw@chromium.org changed reviewers:
+ bajones@chromium.org

[klausw, 2017-01-04 21:57:12]

What do you think of this approach? It should essentially
be a no-op.

I'll add tests for the new GLSurfaceFormat class assuming
we want to go this route.

[klausw, 2017-01-04 21:57:23]

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

[commit-bot: I haz the power, 2017-01-04 21:57:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 22:07:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 22:07:22]

Dry run: Try jobs failed on following builders:
  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_...)

[klausw, 2017-01-04 22:19:50]

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

[commit-bot: I haz the power, 2017-01-04 22:20:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 22:30:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 22:30:19]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[bajones, 2017-01-04 22:31:55]

Generally looks good, the one bit I question is the various places where we
check for IsDefault and then sub in the previously used format. Seems like we
should either force it to the old format all the time or find some way to merge
the two?

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (left):

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface.cc#oldcode363
ui/gl/gl_surface.cc:363: if (!surface->Initialize())
Nit: This function doesn't actually need to change, right? Calling Initialize()
will use a "Default" surface format.

I think I'd prefer to either leave this call alone or get rid of the default
Initialize() call.

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:1206: if (format.IsDefault()) {
This feels weird, though I can certainly see how you arrived here. Seems like
it's probably an error to request a surfaceless surface with anything other than
the default settings. Maybe turn the if into a DCHECK and always override to
SURFACE_SURFACELESS?

[klausw, 2017-01-05 00:55:13]

> the one bit I question is the various places where we
check for IsDefault and then sub in the previously used format. Seems like we
> should either force it to the old format all the time or find some way to
merge
the two?

PTAL, I've changed the logic to do merging, and was able
to get rid of all the legacy formats in the process. Just
need to keep the layout which is now its own separate
set of constants.

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (left):

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface.cc#oldcode363
ui/gl/gl_surface.cc:363: if (!surface->Initialize())
On 2017/01/04 22:31:55, bajones wrote:
> Nit: This function doesn't actually need to change, right? Calling
Initialize()
> will use a "Default" surface format.
> 
> I think I'd prefer to either leave this call alone or get rid of the default
> Initialize() call.

I needed a new Initialize..WithFormat, and I don't want two copies of the code,
so I changed the no-format version to call the new
WithFormat one to avoid things getting out of sync in the future.

I'm not sure what you mean by "default Initialize() call", it's being
called with a format argument.

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/2616723002/diff/1/ui/gl/gl_surface_egl.cc#new...
ui/gl/gl_surface_egl.cc:1206: if (format.IsDefault()) {
On 2017/01/04 22:31:55, bajones wrote:
> This feels weird, though I can certainly see how you arrived here. Seems like
> it's probably an error to request a surfaceless surface with anything other
than
> the default settings. Maybe turn the if into a DCHECK and always override to
> SURFACE_SURFACELESS?

Changed to set surfaceless unconditionally.

[klausw, 2017-01-05 00:55:35]

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

[commit-bot: I haz the power, 2017-01-05 00:56:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 01:16:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 01:16:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[klausw, 2017-01-05 01:25:18]

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

[commit-bot: I haz the power, 2017-01-05 01:25:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 01:48:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 01:48:28]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[klausw, 2017-01-05 01:58:26]

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

[commit-bot: I haz the power, 2017-01-05 01:58:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-01-05 02:20:43]

Description was changed from

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

The old enums are still used as shortcuts for initializing the
GLSurfaceFormat objects, these could be replaced with more
specific builder-stule initialization in the future if needed.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002
which is now forked into https://crrev.com/2586803003 .)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2017-01-05 02:24:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:24:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[klausw, 2017-01-05 02:31:43]

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

[commit-bot: I haz the power, 2017-01-05 02:32:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-01-05 02:37:49]

klausw@chromium.org changed reviewers:
+ jbauman@chromium.org

[klausw, 2017-01-05 02:37:50]

jbauman@chromium.org: This is a followup to revisit
bajones's https://crrev.com/2461803002, splitting it into two
patches. This patch is intended to be a no-op refactor of
how GLSurface::Format works.

(The followup is going to be https://crrev.com/2586803003
which is a rebased version of bajones's earlier patch.)

Please let me know if you have questions. I'm still
working on fixing some issues caught by the trybots for
non-Linux/Android systems that I couldn't test locally,
apologies for the noise in case there's going to be
more followup patchsets to sort these out.

[klausw, 2017-01-05 02:40:39]

klausw@chromium.org changed reviewers:
+ dnicoara@chromium.org

[klausw, 2017-01-05 02:40:40]

dnicoara@chromium.org: Please review changes in ui/ozone, this should be a
no-op.

[commit-bot: I haz the power, 2017-01-05 02:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 02:46:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[klausw, 2017-01-05 03:13:52]

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

[commit-bot: I haz the power, 2017-01-05 03:14:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-01-05 03:30:52]

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

[commit-bot: I haz the power, 2017-01-05 03:31:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 04:26:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 04:26:52]

Dry run: This issue passed the CQ dry run.

[bajones, 2017-01-05 17:16:01]

LGTM now, though I'm not an owner for most of it.

[klausw, 2017-01-05 20:31:25]

Description was changed from

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002
which is now forked into https://crrev.com/2586803003 .)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002
which is now forked into https://crrev.com/2586803003 .)

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[jbauman, 2017-01-06 02:51:30]

https://codereview.chromium.org/2616723002/diff/160001/ui/gl/gl_surface_format.h
File ui/gl/gl_surface_format.h (right):

https://codereview.chromium.org/2616723002/diff/160001/ui/gl/gl_surface_forma...
ui/gl/gl_surface_format.h:59: bool is_default = true;
is_default_, pixel_layout_, is_surfaceless_, etc.

[klausw, 2017-01-06 03:44:40]

https://codereview.chromium.org/2616723002/diff/160001/ui/gl/gl_surface_format.h
File ui/gl/gl_surface_format.h (right):

https://codereview.chromium.org/2616723002/diff/160001/ui/gl/gl_surface_forma...
ui/gl/gl_surface_format.h:59: bool is_default = true;
On 2017/01/06 02:51:30, jbauman wrote:
> is_default_, pixel_layout_, is_surfaceless_, etc.

Oops, done.

[jbauman, 2017-01-06 03:51:37]

lgtm with one nit.

https://codereview.chromium.org/2616723002/diff/180001/ui/gl/gl_surface_format.h
File ui/gl/gl_surface_format.h (right):

https://codereview.chromium.org/2616723002/diff/180001/ui/gl/gl_surface_forma...
ui/gl/gl_surface_format.h:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
Use the new license header style for new files.

[klausw, 2017-01-06 06:21:29]

https://codereview.chromium.org/2616723002/diff/180001/ui/gl/gl_surface_format.h
File ui/gl/gl_surface_format.h (right):

https://codereview.chromium.org/2616723002/diff/180001/ui/gl/gl_surface_forma...
ui/gl/gl_surface_format.h:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
On 2017/01/06 03:51:37, jbauman wrote:
> Use the new license header style for new files.

Done, I used the
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
template which apparently just removes the (c).

[dnicoara, 2017-01-06 14:09:19]

ui/ozone lgtm

[klausw, 2017-01-06 16:27:14]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-01-06 16:27:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
bajones@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2616723002/#ps200001
(title: "Fix copyright notice on new files")

[commit-bot: I haz the power, 2017-01-06 16:27:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 17:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 17:41:58]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[klausw, 2017-01-06 17:46:26]

The CQ bit was checked by klausw@chromium.org

[commit-bot: I haz the power, 2017-01-06 17:47:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 18:48:51]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483724786370250,
"parent_rev": "d526cb76cf018c800027b5c9e238e520dcecb68a", "commit_rev":
"ec8edae43017bf5ea65d3b65edbcfd740af453c7"}

[commit-bot: I haz the power, 2017-01-06 18:49:18]

Description was changed from

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002
which is now forked into https://crrev.com/2586803003 .)

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Refactor GL surface format handling

Replace the GLSurface::Format enum with a proper class in preparation to
supporting types with configurable depth_size / samples / stencil_size.
Doing this in the current enum format would lead to a combinatorial
explosion.

The basic idea is that GetFormat can be used to initialize a new
surface with the same format as a pre-existing surface, and a
IsCompatible predicate is used to check if two formats are
equivalent for the purpose of avoiding BAD_MATCH errors. Code
should no longer be comparing formats to SURFACE_DEFAULT or similar.

Also add a new CreateOffscreenGLSurfaceWithFormat function that can
be used to request a specific surface format.

(These changes were suggested during review of https://crrev.com/2461803002
which is now forked into https://crrev.com/2586803003 .)

BUG=655722
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2616723002
Cr-Commit-Position: refs/heads/master@{#441993}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
==========

[commit-bot: I haz the power, 2017-01-06 18:49:20]

Committed patchset #12 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...