Issue 2270463002: Add OzoneGLImpl interface.

kylechar

Patchset #5 (id:80001) has been deleted

4 years, 3 months ago (2016-08-26 13:01:35 UTC) #1

kylechar

Description was changed from ========== Add OzoneGLImpl interface. Move the Ozone platform GL implementation code ...

4 years, 3 months ago (2016-08-26 15:17:35 UTC) #2

kylechar

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

4 years, 3 months ago (2016-08-26 15:18:30 UTC) #3

kylechar

rjkroege: what do you think of the general concept and review for ui/ozone/*?

4 years, 3 months ago (2016-08-26 15:18:31 UTC) #4

rjkroege

I like the approach. Some comments / ideas that we could discuss. https://codereview.chromium.org/2270463002/diff/120001/ui/gl/BUILD.gn File ui/gl/BUILD.gn ...

4 years, 3 months ago (2016-08-26 19:55:53 UTC) #5

kylechar

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/BUILD.gn File ui/gl/BUILD.gn (right): https://codereview.chromium.org/2270463002/diff/120001/ui/gl/BUILD.gn#newcode316 ui/gl/BUILD.gn:316: deps += [ "//ui/ozone" ] On 2016/08/26 19:55:53, rjkroege ...

4 years, 3 months ago (2016-08-29 15:38:15 UTC) #6

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/BUILD.gn
File ui/gl/BUILD.gn (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/BUILD.gn#newcode316
ui/gl/BUILD.gn:316: deps += [ "//ui/ozone" ]
On 2016/08/26 19:55:53, rjkroege wrote:
> Would it be too hard to narrow the dependency?. Can we only depend on
> ui/ozone/public here?

Well, the DEPS only allows ui/ozone/public for #include statements. There is no
//ui/ozone/public target but the //ui/ozone target only exposes headers in
//ui/ozone/public so it's essentially equivalent? 

This is also only to perform the GPU initialization for tests and might not be
necessary if that was done differently.

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/gl_factory_...
File ui/gl/init/gl_factory_ozone.cc (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/gl_factory_...
ui/gl/init/gl_factory_ozone.cc:63: ui::OzonePlatform::InitializeForGPU();
On 2016/08/26 19:55:53, rjkroege wrote:
> in my opinion, this is not the right place to init Ozone's GPU thread support
> for at least two reasons.
> 
> 1. in a Vulkan only world, we might not init GL. So we should Initialize at a
> higher level and not as a side-effect of setting up GL
> 
> 2. Because Ozone creates threads, ozone init has ordering constraints with the
> establishing of a sandbox. We might want GetAllowedGLImplementations to not be
> bound by these constraints. In particular, in Mus, we want to call
> InitializeForGPU on the CC thread and not the GPU thread like it is in the
> Chrome GPU project.

I'm not the biggest fan of this either but this is the first line of Ozone
specific code that gets called during GL initialization. I could move
OzonePlatform::InitializeForGPU into gl_factory.cc with an #ifdef but that
doesn't fix the thread problems you brought up.

Where were you thinking this call could be?

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/ozone_util.cc
File ui/gl/init/ozone_util.cc (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/ozone_util....
ui/gl/init/ozone_util.cc:11: ui::SurfaceFactoryOzone* GetSurfaceFactory() {
On 2016/08/26 19:55:53, rjkroege wrote:
> you could make a lot of this inline

Done.

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/ozone_util.h
File ui/gl/init/ozone_util.h (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/ozone_util....
ui/gl/init/ozone_util.h:13: namespace gl {
On 2016/08/26 19:55:53, rjkroege wrote:
> Can you remind me of the layering? I think it should go like this?
> 
> ui/gl -> ui/ozone -> gl
> 
> yes?

Yep, that's exactly it.

https://codereview.chromium.org/2270463002/diff/120001/ui/ozone/public/gl_ozo...
File ui/ozone/public/gl_ozone.h (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/ozone/public/gl_ozo...
ui/ozone/public/gl_ozone.h:27: class OZONE_BASE_EXPORT GLOzone {
On 2016/08/26 19:55:53, rjkroege wrote:
> aside: we will need a VulkanOzone too at some point.

sgtm.

https://codereview.chromium.org/2270463002/diff/120001/ui/ozone/public/surfac...
File ui/ozone/public/surface_factory_ozone.h (right):

https://codereview.chromium.org/2270463002/diff/120001/ui/ozone/public/surfac...
ui/ozone/public/surface_factory_ozone.h:64: virtual
std::vector<gl::GLImplementation> GetAllowedGLImplementations();
On 2016/08/26 19:55:53, rjkroege wrote:
> How hard to you think that it would be move this into GLOzone?

Not super hard but I don't think it's worth it. This method can't just be moved
into GLOzone as there will be potentially many GLOzone instances for each Ozone
platform. Each GLOzone implements one (or maybe multiple, see the
gl_factory_mac.cc..) of the GL implementation enums. If we knew what
GLImplementation corresponded to what GLOzone object then we can create a lookup
table of sorts.

I originally coded it so that each GLOzone has a method that returns the
GLImplementation it is for. SurfaceFactoryOzone then builds an
unordered_map<GLImplementation, GLOzone> based on that and can generate a list
of GLImplementations from that. It adds a bunch of a complexity to save a tiny
bit of boilerplate for each Ozone platform, so I scrapped it in favour of this.

rjkroege

Thanks for the explanations. lgtm. https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/gl_factory_ozone.cc File ui/gl/init/gl_factory_ozone.cc (right): https://codereview.chromium.org/2270463002/diff/120001/ui/gl/init/gl_factory_ozone.cc#newcode63 ui/gl/init/gl_factory_ozone.cc:63: ui::OzonePlatform::InitializeForGPU(); On 2016/08/29 15:38:14, ...

4 years, 3 months ago (2016-08-30 18:47:31 UTC) #7

kylechar

Description was changed from ========== Add GLOzone interface. Move the Ozone platform GL implementation code ...

4 years, 3 months ago (2016-08-31 14:10:21 UTC) #8

Description was changed from

==========
Add GLOzone interface.

Move the Ozone platform GL implementation code into a single interface.
Right now the code to implement EGL for an Ozone platform is located
part in //ui/gl/init and part in SurfaceFactoryOzone implementation.

The current approach exposes EGL specific concepts in the shared Ozone
code. This works fine currently because EGL is relatively platform
agnostic. This gets much messier for GLX because it is not platform
agnostic.

Move the existing shared EGL code into an abstract class GLOzoneGL that
implements GLOzone. Each Ozone platform can then finish the
implementation. This has a nice side effect that SurfaceFactoryOzone no
longer has to expose EGL concepts.

Only the Ozone X11 platform is updated initially. The GBM, cast and
wayland Ozone platforms will need to be updated to use the new interface
in a subsequent CL.

BUG=640613
==========

to

==========
Add GLOzone interface.

Move the Ozone platform GL implementation code into a single interface.
Right now the code to implement EGL for an Ozone platform is located
part in //ui/gl/init and part in SurfaceFactoryOzone implementation.

The current approach exposes EGL specific concepts in the shared Ozone
code. This works fine currently because EGL is relatively platform
agnostic. This gets much messier for GLX because it is not platform
agnostic.

Move the existing shared EGL code into an abstract class GLOzoneGL that
implements GLOzone. Each Ozone platform can then finish the
implementation. This has a nice side effect that SurfaceFactoryOzone no
longer has to expose EGL concepts.

Only the Ozone X11 platform is updated initially. The GBM, cast and
wayland Ozone platforms will need to be updated to use the new interface
in a subsequent CL.

BUG=640613
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
==========

kylechar

kylechar@chromium.org changed reviewers: + piman@chromium.org

4 years, 3 months ago (2016-08-31 16:10:49 UTC) #10

kylechar

+piman for ui/gl/*. I'm not super happy about how OzonePlatform::InitializeForGPU() is called as a side ...

4 years, 3 months ago (2016-08-31 16:10:50 UTC) #11

piman

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc File ui/gl/init/gl_initializer_ozone.cc (left): https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc#oldcode61 ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU(); nit: because this moves to >1 location, is ...

4 years, 3 months ago (2016-08-31 20:05:10 UTC) #12

kylechar

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc File ui/gl/init/gl_initializer_ozone.cc (left): https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc#oldcode61 ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU(); On 2016/08/31 20:05:09, piman wrote: > nit: because ...

4 years, 3 months ago (2016-09-01 13:57:21 UTC) #13

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
File ui/gl/init/gl_initializer_ozone.cc (left):

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU();
On 2016/08/31 20:05:09, piman wrote:
> nit: because this moves to >1 location, is there a way to add a DCHECK
somewhere
> to make sure we didn't miss any?

I can add an explicit DCHECK(OzonePlatform::GetInstance()) at the start of
InitializeGLOneOffPlatform() and InitializeStaticGLBindings() for sure.

Realistically it doesn't make much difference because
OzonePlatform::GetInstance() has it's own DCHECK, so that will be what fails. It
would also be what fails now on the first HasGLOzone() call in both of those
methods.

Another option is to move OzonePlatform::InitializeForGPU() to gpu_main.cc. It's
conceptually much nicer for Chrome because initializing OzonePlatform as part of
GL initialization seems flawed. What if we don't want to initialize GL (eg.
Vulkan) then OzonePlatform doesn't get initialized. The big problem with this is
there are ~10 unit tests that also need to initialize OzonePlatform in their
main or test fixture.

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util.h
File ui/gl/init/ozone_util.h (right):

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util....
ui/gl/init/ozone_util.h:16: inline ui::SurfaceFactoryOzone* GetSurfaceFactory()
{
On 2016/08/31 20:05:09, piman wrote:
> nit: gl::GetSurfaceFactory doesn't make it clear that it's specific to Ozone.
> Could we make it GetOzoneSurfaceFactory, or gl::ozone::GetSurfaceFactory, or
> something?

Done.

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util....
ui/gl/init/ozone_util.h:22: return GetSurfaceFactory()->GetGLOzone(impl) !=
nullptr;
On 2016/08/31 20:05:09, piman wrote:
> Is the plan to have this always be true? If not, could we make a default one,
> and move all the logic in gl_factory_ozone and gl_initializer_ozone that
depends
> on !HasGLOzone to that default GLOzone?

I think that yes, it would always be true. I could add a different way to check
if a GLOzone exists for the GLImplementation but it seems unnecessary (more
boilerplate to write for each Ozone platform). Also, for example, if
impl=kGLImplementationAppleGL then you probably don't want to return a default
because something is super wrong.

Although you're absolutely right I could move the osmesa and mock GL code into
GLOzoneOsmesa and GLOzoneMock respectively. That way each of the functions in
gl_ozone_initializer/gl_ozone_factory would look like:

bool InitializeGLOneOffPlatform() {
  return GetGLOzone()->InitializeGLOneOffPlatform();
}

That would be a nice change.

piman

lgtm https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc File ui/gl/init/gl_initializer_ozone.cc (left): https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc#oldcode61 ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU(); On 2016/09/01 13:57:21, kylechar wrote: > On ...

4 years, 3 months ago (2016-09-01 17:43:04 UTC) #14

lgtm

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
File ui/gl/init/gl_initializer_ozone.cc (left):

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU();
On 2016/09/01 13:57:21, kylechar wrote:
> On 2016/08/31 20:05:09, piman wrote:
> > nit: because this moves to >1 location, is there a way to add a DCHECK
> somewhere
> > to make sure we didn't miss any?
> 
> I can add an explicit DCHECK(OzonePlatform::GetInstance()) at the start of
> InitializeGLOneOffPlatform() and InitializeStaticGLBindings() for sure.
> 
> Realistically it doesn't make much difference because
> OzonePlatform::GetInstance() has it's own DCHECK, so that will be what fails.
It
> would also be what fails now on the first HasGLOzone() call in both of those
> methods.

Ok, if it's redundant, then nbd.

> Another option is to move OzonePlatform::InitializeForGPU() to gpu_main.cc.
It's
> conceptually much nicer for Chrome because initializing OzonePlatform as part
of
> GL initialization seems flawed. What if we don't want to initialize GL (eg.
> Vulkan) then OzonePlatform doesn't get initialized. The big problem with this
is
> there are ~10 unit tests that also need to initialize OzonePlatform in their
> main or test fixture.

It may make sense to have it both in gpu_main.cc and GLSurfaceTestSupport,
because the latter is intended to initialize all the platform requirements for
GPU, indeed to avoid having to maintain 10ish test suites. Up to you.

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util.h
File ui/gl/init/ozone_util.h (right):

https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util....
ui/gl/init/ozone_util.h:22: return GetSurfaceFactory()->GetGLOzone(impl) !=
nullptr;
On 2016/09/01 13:57:21, kylechar wrote:
> On 2016/08/31 20:05:09, piman wrote:
> > Is the plan to have this always be true? If not, could we make a default
one,
> > and move all the logic in gl_factory_ozone and gl_initializer_ozone that
> depends
> > on !HasGLOzone to that default GLOzone?
> 
> I think that yes, it would always be true. I could add a different way to
check
> if a GLOzone exists for the GLImplementation but it seems unnecessary (more
> boilerplate to write for each Ozone platform). Also, for example, if
> impl=kGLImplementationAppleGL then you probably don't want to return a default
> because something is super wrong.
> 
> Although you're absolutely right I could move the osmesa and mock GL code into
> GLOzoneOsmesa and GLOzoneMock respectively. That way each of the functions in
> gl_ozone_initializer/gl_ozone_factory would look like:
> 
> bool InitializeGLOneOffPlatform() {
>   return GetGLOzone()->InitializeGLOneOffPlatform();
> }
> 
> That would be a nice change.

Right, I would like that in the end state we don't have half of the logic in
ui/gl/init and the other half in ui/ozone - it's harder to follow and to
maintain.

kylechar

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

4 years, 3 months ago (2016-09-01 18:11:28 UTC) #15

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-01 18:11:48 UTC) #16

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-01 19:36:24 UTC) #17

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-01 19:36:25 UTC) #18

kylechar

On 2016/09/01 17:43:04, piman wrote: > lgtm > > https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initializer_ozone.cc > File ui/gl/init/gl_initializer_ozone.cc (left): > ...

4 years, 3 months ago (2016-09-01 20:43:46 UTC) #19

On 2016/09/01 17:43:04, piman wrote:
> lgtm
> 
>
https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
> File ui/gl/init/gl_initializer_ozone.cc (left):
> 
>
https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/gl_initiali...
> ui/gl/init/gl_initializer_ozone.cc:61: ui::OzonePlatform::InitializeForGPU();
> On 2016/09/01 13:57:21, kylechar wrote:
> > On 2016/08/31 20:05:09, piman wrote:
> > > nit: because this moves to >1 location, is there a way to add a DCHECK
> > somewhere
> > > to make sure we didn't miss any?
> > 
> > I can add an explicit DCHECK(OzonePlatform::GetInstance()) at the start of
> > InitializeGLOneOffPlatform() and InitializeStaticGLBindings() for sure.
> > 
> > Realistically it doesn't make much difference because
> > OzonePlatform::GetInstance() has it's own DCHECK, so that will be what
fails.
> It
> > would also be what fails now on the first HasGLOzone() call in both of those
> > methods.
> 
> Ok, if it's redundant, then nbd.
> 
> > Another option is to move OzonePlatform::InitializeForGPU() to gpu_main.cc.
> It's
> > conceptually much nicer for Chrome because initializing OzonePlatform as
part
> of
> > GL initialization seems flawed. What if we don't want to initialize GL (eg.
> > Vulkan) then OzonePlatform doesn't get initialized. The big problem with
this
> is
> > there are ~10 unit tests that also need to initialize OzonePlatform in their
> > main or test fixture.
> 
> It may make sense to have it both in gpu_main.cc and GLSurfaceTestSupport,
> because the latter is intended to initialize all the platform requirements for
> GPU, indeed to avoid having to maintain 10ish test suites. Up to you.

I'll look into this in a follow up CL.

> https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util.h
> File ui/gl/init/ozone_util.h (right):
> 
>
https://codereview.chromium.org/2270463002/diff/200001/ui/gl/init/ozone_util....
> ui/gl/init/ozone_util.h:22: return GetSurfaceFactory()->GetGLOzone(impl) !=
> nullptr;
> On 2016/09/01 13:57:21, kylechar wrote:
> > On 2016/08/31 20:05:09, piman wrote:
> > > Is the plan to have this always be true? If not, could we make a default
> one,
> > > and move all the logic in gl_factory_ozone and gl_initializer_ozone that
> > depends
> > > on !HasGLOzone to that default GLOzone?
> > 
> > I think that yes, it would always be true. I could add a different way to
> check
> > if a GLOzone exists for the GLImplementation but it seems unnecessary (more
> > boilerplate to write for each Ozone platform). Also, for example, if
> > impl=kGLImplementationAppleGL then you probably don't want to return a
default
> > because something is super wrong.
> > 
> > Although you're absolutely right I could move the osmesa and mock GL code
into
> > GLOzoneOsmesa and GLOzoneMock respectively. That way each of the functions
in
> > gl_ozone_initializer/gl_ozone_factory would look like:
> > 
> > bool InitializeGLOneOffPlatform() {
> >   return GetGLOzone()->InitializeGLOneOffPlatform();
> > }
> > 
> > That would be a nice change.
> 
> Right, I would like that in the end state we don't have half of the logic in
> ui/gl/init and the other half in ui/ozone - it's harder to follow and to
> maintain.

I added crbug.com/643368 to track this.

kylechar

The patchset sent to the CQ was uploaded after l-g-t-m from rjkroege@chromium.org Link to the ...

4 years, 3 months ago (2016-09-01 20:43:56 UTC) #21

commit-bot: I haz the power

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

4 years, 3 months ago (2016-09-01 20:44:51 UTC) #22

commit-bot: I haz the power

Description was changed from ========== Add GLOzone interface. Move the Ozone platform GL implementation code ...

4 years, 3 months ago (2016-09-01 20:56:03 UTC) #24

Message was sent while issue was closed.

Description was changed from

==========
Add GLOzone interface.

Move the Ozone platform GL implementation code into a single interface.
Right now the code to implement EGL for an Ozone platform is located
part in //ui/gl/init and part in SurfaceFactoryOzone implementation.

The current approach exposes EGL specific concepts in the shared Ozone
code. This works fine currently because EGL is relatively platform
agnostic. This gets much messier for GLX because it is not platform
agnostic.

Move the existing shared EGL code into an abstract class GLOzoneGL that
implements GLOzone. Each Ozone platform can then finish the
implementation. This has a nice side effect that SurfaceFactoryOzone no
longer has to expose EGL concepts.

Only the Ozone X11 platform is updated initially. The GBM, cast and
wayland Ozone platforms will need to be updated to use the new interface
in a subsequent CL.

BUG=640613
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

==========
Add GLOzone interface.

Move the Ozone platform GL implementation code into a single interface.
Right now the code to implement EGL for an Ozone platform is located
part in //ui/gl/init and part in SurfaceFactoryOzone implementation.

The current approach exposes EGL specific concepts in the shared Ozone
code. This works fine currently because EGL is relatively platform
agnostic. This gets much messier for GLX because it is not platform
agnostic.

Move the existing shared EGL code into an abstract class GLOzoneGL that
implements GLOzone. Each Ozone platform can then finish the
implementation. This has a nice side effect that SurfaceFactoryOzone no
longer has to expose EGL concepts.

Only the Ozone X11 platform is updated initially. The GBM, cast and
wayland Ozone platforms will need to be updated to use the new interface
in a subsequent CL.

BUG=640613
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

Committed: https://crrev.com/036945276e54bd6d98d3e51ccfec21c56dc80b61
Cr-Commit-Position: refs/heads/master@{#416045}
==========

commit-bot: I haz the power

4 years, 3 months ago (2016-09-01 20:56:04 UTC) #25

Message was sent while issue was closed.

Patchset 10 (id:??) landed as
https://crrev.com/036945276e54bd6d98d3e51ccfec21c56dc80b61
Cr-Commit-Position: refs/heads/master@{#416045}

Issue 2270463002: Add OzoneGLImpl interface. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Naming/documentation. #

Patch Set 3 : Fix tests. #

Patch Set 4 : Actually fix test. #

Patch Set 5 : Fix GN check. #

Patch Set 6 : Rename. #

Patch Set 7 : Fix for comments. #

Patch Set 8 : Change initialization. #

Patch Set 9 : Undo initialization changes. #

Patch Set 10 : Address comments. #

Messages