ozone/platform/wayland: Implement SurfaceOzoneEGL

ozone/platform/wayland: Implement SurfaceOzoneEGL

BUG=578890
Committed: https://crrev.com/bc96672e1982f3a86e4058a2f3f8609b9bc592c8
Cr-Commit-Position: refs/heads/master@{#386765}

[Michael Forney, 2016-02-24 07:36:01]

forney@google.com changed reviewers:
+ rjkroege@chromium.org, spang@chromium.org

[Michael Forney, 2016-02-24 07:36:01]

I ran into a few issues with this.

`mojo_runner mojo:mash_shell` does not start at all unless I am running with
swrast egl_gallium (removed from new mesa, but has pbuffer support?), or I
change
https://chromium.googlesource.com/chromium/src/+/2573ce7d143bee9c1f88dac18b92...
from `gfx::Size(1, 1)` to `gfx::Size()` (to prevent pbuffer usage). It appears
that mesa does not support pbuffers in wayland with egl_dri2.

I found http://crrev.com/1343513002 which seems closely related. What is this
external mojo repository? Will this commit eventually make its way into the
chromium command_buffer_driver.cc?

Even after making the gfx::Size change, I still get varied results when I run
mash_shell. Sometimes it crashes, sometimes I get an empty window containing
only the quick launch dialog, and sometimes I get the task bar and background
too. However, I observe the same behavior with --ozone-platform=x11, so I am
assuming that this is because mus with ozone is still WIP.

[dnicoara, 2016-02-24 15:14:29]

On 2016/02/24 07:36:01, Michael Forney wrote:
> I ran into a few issues with this.
> 
> `mojo_runner mojo:mash_shell` does not start at all unless I am running with
> swrast egl_gallium (removed from new mesa, but has pbuffer support?), or I
> change
>
https://chromium.googlesource.com/chromium/src/+/2573ce7d143bee9c1f88dac18b92...
> from `gfx::Size(1, 1)` to `gfx::Size()` (to prevent pbuffer usage). It appears
> that mesa does not support pbuffers in wayland with egl_dri2.
> 
> I found http://crrev.com/1343513002 which seems closely related. What is this
> external mojo repository? Will this commit eventually make its way into the
> chromium command_buffer_driver.cc?

Quick drive-by since I had to deal with this for Ozone.

You should pass a gfx::Size() when creating an offscreen surface (in
CommandBufferDriver). That'll allow the use of the surfaceless context extension
and won't use pbuffers.

NOTE: Even if surfaceless contexts aren't supported and the
GLSurface::CreateOffscreenGLSurface() allocation falls-back to pbuffer, the
PbufferGLSurface constructor will update the size to gfx::Size(1,1) such that
the allocation doesn't fail. 

The real issue here is that some of the drivers don't support (0, 0) sized
allocation. All offscreen surfaces allocated by
GLSurface::CreateOffscreenGLSurface() either support a (0, 0) size or they will
update it to (1, 1) to get around driver bugs, so callers of
GLSurface::CreateOffscreenGLSurface() don't need to worry about this issue.

> 
> Even after making the gfx::Size change, I still get varied results when I run
> mash_shell. Sometimes it crashes, sometimes I get an empty window containing
> only the quick launch dialog, and sometimes I get the task bar and background
> too. However, I observe the same behavior with --ozone-platform=x11, so I am
> assuming that this is because mus with ozone is still WIP.

[spang, 2016-02-24 16:12:11]

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
Does it need to be configurable (is the library every missing?)

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_surface_factory.cc (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_surface_factory.cc:160: DCHECK(display_);
It's not useful to DCHECK if you immediately dereference. Just remove the check.

[Michael Forney, 2016-02-24 22:46:41]

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
On 2016/02/24 16:12:11, spang wrote:
> Does it need to be configurable (is the library every missing?)

It could be missing if the user has no mesa or built mesa without the wayland
EGL platform.

If you are only going to run chromium on a compositor that doesn't support EGL
(for example, weston's RDP backend), I don't see any reason to require
wayland-egl.

Then again, I suspect that chromium has a much deeper dependency on GL/EGL, and
software rendering support is lacking, so maybe adding an option here would be
futile.

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/wayland_surface_factory.cc (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/wayland_surface_factory.cc:160: DCHECK(display_);
On 2016/02/24 16:12:11, spang wrote:
> It's not useful to DCHECK if you immediately dereference. Just remove the
check.

Done.

[spang, 2016-02-25 00:37:31]

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
On 2016/02/24 22:46:41, Michael Forney wrote:
> On 2016/02/24 16:12:11, spang wrote:
> > Does it need to be configurable (is the library every missing?)
> 
> It could be missing if the user has no mesa or built mesa without the wayland
> EGL platform.
> 
> If you are only going to run chromium on a compositor that doesn't support EGL
> (for example, weston's RDP backend), I don't see any reason to require
> wayland-egl.
> 
> Then again, I suspect that chromium has a much deeper dependency on GL/EGL,
and
> software rendering support is lacking, so maybe adding an option here would be
> futile.

Users don't build chrome. Whatever features there are must work in the default
build.

Library dependencies of linux chrome that are "optional" have to be loaded via
dlopen. This includes EGL.

[Michael Forney, 2016-04-01 18:19:33]

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
File ui/ozone/platform/wayland/BUILD.gn (right):

https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
On 2016/02/25 00:37:30, spang wrote:
> On 2016/02/24 22:46:41, Michael Forney wrote:
> > On 2016/02/24 16:12:11, spang wrote:
> > > Does it need to be configurable (is the library every missing?)
> > 
> > It could be missing if the user has no mesa or built mesa without the
wayland
> > EGL platform.
> > 
> > If you are only going to run chromium on a compositor that doesn't support
EGL
> > (for example, weston's RDP backend), I don't see any reason to require
> > wayland-egl.
> > 
> > Then again, I suspect that chromium has a much deeper dependency on GL/EGL,
> and
> > software rendering support is lacking, so maybe adding an option here would
be
> > futile.
> 
> Users don't build chrome. Whatever features there are must work in the default
> build.
> 
> Library dependencies of linux chrome that are "optional" have to be loaded via
> dlopen. This includes EGL.

On 2016/02/25 00:37:31, spang wrote:
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> File ui/ozone/platform/wayland/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
> On 2016/02/24 22:46:41, Michael Forney wrote:
> > On 2016/02/24 16:12:11, spang wrote:
> > > Does it need to be configurable (is the library every missing?)
> > 
> > It could be missing if the user has no mesa or built mesa without the
wayland
> > EGL platform.
> > 
> > If you are only going to run chromium on a compositor that doesn't support
EGL
> > (for example, weston's RDP backend), I don't see any reason to require
> > wayland-egl.
> > 
> > Then again, I suspect that chromium has a much deeper dependency on GL/EGL,
> and
> > software rendering support is lacking, so maybe adding an option here would
be
> > futile.
> 
> Users don't build chrome.

Users build chromium (in source-based distributions like gentoo and exherbo).

> Whatever features there are must work in the default
> build.

I changed the default to true, if that addresses your concern.

> Library dependencies of linux chrome that are "optional" have to be loaded via
> dlopen. This includes EGL.

As far as I can tell, this isn't the case for things like

  use_glib
  use_alsa
  use_cups
  use_pulseaudio
  use_xkbcommon

I disagree with this practice of adding hard dependencies on things that many
users have, even if the browser can run fine without them. However, if you still
feel strongly that there shouldn't be an option, I will remove it.

[spang, 2016-04-01 18:58:28]

On 2016/04/01 18:19:33, Michael Forney wrote:
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> File ui/ozone/platform/wayland/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
> On 2016/02/25 00:37:30, spang wrote:
> > On 2016/02/24 22:46:41, Michael Forney wrote:
> > > On 2016/02/24 16:12:11, spang wrote:
> > > > Does it need to be configurable (is the library every missing?)
> > > 
> > > It could be missing if the user has no mesa or built mesa without the
> wayland
> > > EGL platform.
> > > 
> > > If you are only going to run chromium on a compositor that doesn't support
> EGL
> > > (for example, weston's RDP backend), I don't see any reason to require
> > > wayland-egl.
> > > 
> > > Then again, I suspect that chromium has a much deeper dependency on
GL/EGL,
> > and
> > > software rendering support is lacking, so maybe adding an option here
would
> be
> > > futile.
> > 
> > Users don't build chrome. Whatever features there are must work in the
default
> > build.
> > 
> > Library dependencies of linux chrome that are "optional" have to be loaded
via
> > dlopen. This includes EGL.
> 
> On 2016/02/25 00:37:31, spang wrote:
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > File ui/ozone/platform/wayland/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
> > On 2016/02/24 22:46:41, Michael Forney wrote:
> > > On 2016/02/24 16:12:11, spang wrote:
> > > > Does it need to be configurable (is the library every missing?)
> > > 
> > > It could be missing if the user has no mesa or built mesa without the
> wayland
> > > EGL platform.
> > > 
> > > If you are only going to run chromium on a compositor that doesn't support
> EGL
> > > (for example, weston's RDP backend), I don't see any reason to require
> > > wayland-egl.
> > > 
> > > Then again, I suspect that chromium has a much deeper dependency on
GL/EGL,
> > and
> > > software rendering support is lacking, so maybe adding an option here
would
> be
> > > futile.
> > 
> > Users don't build chrome.
> 
> Users build chromium (in source-based distributions like gentoo and exherbo).
> 
> > Whatever features there are must work in the default
> > build.
> 
> I changed the default to true, if that addresses your concern.
> 
> > Library dependencies of linux chrome that are "optional" have to be loaded
via
> > dlopen. This includes EGL.
> 
> As far as I can tell, this isn't the case for things like
> 
>   use_glib
>   use_alsa
>   use_cups
>   use_pulseaudio
>   use_xkbcommon
> 
> I disagree with this practice of adding hard dependencies on things that many
> users have, even if the browser can run fine without them. However, if you
still
> feel strongly that there shouldn't be an option, I will remove it.

Alrigh

[spang, 2016-04-01 19:05:02]

On 2016/04/01 18:58:28, spang wrote:
> On 2016/04/01 18:19:33, Michael Forney wrote:
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > File ui/ozone/platform/wayland/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
> > On 2016/02/25 00:37:30, spang wrote:
> > > On 2016/02/24 22:46:41, Michael Forney wrote:
> > > > On 2016/02/24 16:12:11, spang wrote:
> > > > > Does it need to be configurable (is the library every missing?)
> > > > 
> > > > It could be missing if the user has no mesa or built mesa without the
> > wayland
> > > > EGL platform.
> > > > 
> > > > If you are only going to run chromium on a compositor that doesn't
support
> > EGL
> > > > (for example, weston's RDP backend), I don't see any reason to require
> > > > wayland-egl.
> > > > 
> > > > Then again, I suspect that chromium has a much deeper dependency on
> GL/EGL,
> > > and
> > > > software rendering support is lacking, so maybe adding an option here
> would
> > be
> > > > futile.
> > > 
> > > Users don't build chrome. Whatever features there are must work in the
> default
> > > build.
> > > 
> > > Library dependencies of linux chrome that are "optional" have to be loaded
> via
> > > dlopen. This includes EGL.
> > 
> > On 2016/02/25 00:37:31, spang wrote:
> > >
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > > File ui/ozone/platform/wayland/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1704243003/diff/80001/ui/ozone/platform/wayla...
> > > ui/ozone/platform/wayland/BUILD.gn:10: use_wayland_egl = false
> > > On 2016/02/24 22:46:41, Michael Forney wrote:
> > > > On 2016/02/24 16:12:11, spang wrote:
> > > > > Does it need to be configurable (is the library every missing?)
> > > > 
> > > > It could be missing if the user has no mesa or built mesa without the
> > wayland
> > > > EGL platform.
> > > > 
> > > > If you are only going to run chromium on a compositor that doesn't
support
> > EGL
> > > > (for example, weston's RDP backend), I don't see any reason to require
> > > > wayland-egl.
> > > > 
> > > > Then again, I suspect that chromium has a much deeper dependency on
> GL/EGL,
> > > and
> > > > software rendering support is lacking, so maybe adding an option here
> would
> > be
> > > > futile.
> > > 
> > > Users don't build chrome.
> > 
> > Users build chromium (in source-based distributions like gentoo and
exherbo).
> > 
> > > Whatever features there are must work in the default
> > > build.
> > 
> > I changed the default to true, if that addresses your concern.
> > 
> > > Library dependencies of linux chrome that are "optional" have to be loaded
> via
> > > dlopen. This includes EGL.
> > 
> > As far as I can tell, this isn't the case for things like
> > 
> >   use_glib
> >   use_alsa
> >   use_cups
> >   use_pulseaudio
> >   use_xkbcommon
> > 
> > I disagree with this practice of adding hard dependencies on things that
many
> > users have, even if the browser can run fine without them. However, if you
> still
> > feel strongly that there shouldn't be an option, I will remove it.
> 
> Alrigh

Oops.

Alright, lgtm.

I think it's conceptually wrong to conflate the user with the distributor.
Gentoo is an extremely unusual case and not something we design for. The user is
almost never involved in setting build time knobs.

So I don't think it's as impactful as you might think to be able to turn off
wayland-egl. Distributions that want wayland will almost always want support for
EGL. You're compiling out a single class with here; the majority of GLES support
is still built into the binary and I severely doubt we'd ever add a knob to
build a software-only chromium.

[Michael Forney, 2016-04-12 01:00:45]

forney@google.com changed reviewers:
+ mmoss@chromium.org

[Michael Forney, 2016-04-12 01:00:46]

Adding mmoss for build/linux/OWNERS.

[Michael Moss, 2016-04-12 16:47:27]

lgtm

[rjkroege, 2016-04-12 17:42:12]

lgtm if you're waiting.

[Michael Forney, 2016-04-12 17:46:01]

The CQ bit was checked by forney@google.com

[commit-bot: I haz the power, 2016-04-12 17:46:14]

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

[commit-bot: I haz the power, 2016-04-12 19:38:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-12 19:40:57]

Description was changed from

==========
ozone/platform/wayland: Implement SurfaceOzoneEGL

BUG=578890
==========

to

==========
ozone/platform/wayland: Implement SurfaceOzoneEGL

BUG=578890

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

[commit-bot: I haz the power, 2016-04-12 19:40:58]

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