973227 - Use texture from pixmap with X11 and OpenGL

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Tom S [:evilpie]]

Attachment: texturehost-ogl

Well, I don't actually understand much about OpenGL, but I have some understanding on how the TextureHostX11 works. Nicolas mentioned that we should implement "texture from pixmap", so I started figuring out what it is. From there I just copy pasted the GrallocTextureHost code, removed everything that looked unnecessary and added some code from TextureHostX11 and *ding* we've got ourselves an almost working TextureHost implementation. I haven't tested this much yet, but there is some obvious issue, that is for some reason the colors are inverted.

Comment 1

[Tom S [:evilpie]]

Attachment: wip

I fixed the color problem and cleaned up some stuff.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 8376760 [details] [diff] [review]
wip

Review of attachment 8376760 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall, there is just the question of merging the two X11 hosts or having two descriptor types (see comment below) to sort out.

::: gfx/layers/composite/TextureHost.cpp
@@ +183,5 @@
>          return CreateTextureHostBasic(aDesc, aDeallocator, aFlags);
>        }
>  #ifdef MOZ_X11
>      case SurfaceDescriptor::TSurfaceDescriptorX11:
> +      if (Compositor::GetBackend() == LayersBackend::LAYERS_OPENGL) {

We are in the process of removing Compsoitor::GetBackend() because it doesn't work when e10s requires to have software and accelerated backend side by side (software for tooltips and accelerated for the rest). So it'd be best not to add more calls to it.

This also means that we can only pick the backend type depending on the SurfaceDescriptor type. If several backends have a common surface descriptor we should have only one backend-independent TextureClient/Host pair for the SurfaceDescriptor type, and move the backend specific parts to TextureSource (like BufferTextureHost).
Otherwise have a separate surface descriptor type for X11 software and X11 gl

::: gfx/layers/opengl/X11TextureHostOGL.cpp
@@ +18,5 @@
> +
> +static inline SurfaceFormat
> +ContentTypeToSurfaceFormat(gfxContentType type)
> +{
> +  // Use wrong format on purpose? WTF?

what do you mean?

Comment 3

[Tom S [:evilpie]]

(In reply to Nicolas Silva [:nical] from comment #2)
> Comment on attachment 8376760 [details] [diff] [review]
> wip
> 
> Review of attachment 8376760 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall, there is just the question of merging the two X11 hosts
> or having two descriptor types (see comment below) to sort out.
> 
> ::: gfx/layers/composite/TextureHost.cpp
> @@ +183,5 @@
> >          return CreateTextureHostBasic(aDesc, aDeallocator, aFlags);
> >        }
> >  #ifdef MOZ_X11
> >      case SurfaceDescriptor::TSurfaceDescriptorX11:
> > +      if (Compositor::GetBackend() == LayersBackend::LAYERS_OPENGL) {
> 
> We are in the process of removing Compsoitor::GetBackend() because it
> doesn't work when e10s requires to have software and accelerated backend
> side by side (software for tooltips and accelerated for the rest). So it'd
> be best not to add more calls to it.
> 
> This also means that we can only pick the backend type depending on the
> SurfaceDescriptor type. If several backends have a common surface descriptor
> we should have only one backend-independent TextureClient/Host pair for the
> SurfaceDescriptor type, and move the backend specific parts to TextureSource
> (like BufferTextureHost).
> Otherwise have a separate surface descriptor type for X11 software and X11 gl
> 
The TextureHost implementation are basically identical, so it seems like a good idea to share that and just have a different TextureSource implementation. However it's not clear to me on how to decide when to use which.
> ::: gfx/layers/opengl/X11TextureHostOGL.cpp
> @@ +18,5 @@
> > +
> > +static inline SurfaceFormat
> > +ContentTypeToSurfaceFormat(gfxContentType type)
> > +{
> > +  // Use wrong format on purpose? WTF?
> 
> what do you mean?
I had to switch the format around otherwise I would get wrong colors, compare to https://hg.mozilla.org/mozilla-central/diff/ab19291191fe/gfx/layers/basic/TextureHostX11.cpp#l1.20.

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to Tom Schuster [:evilpie] from comment #3)
> > ::: gfx/layers/opengl/X11TextureHostOGL.cpp
> > @@ +18,5 @@
> > > +
> > > +static inline SurfaceFormat
> > > +ContentTypeToSurfaceFormat(gfxContentType type)
> > > +{
> > > +  // Use wrong format on purpose? WTF?
> > 
> > what do you mean?
> I had to switch the format around otherwise I would get wrong colors,
> compare to
> https://hg.mozilla.org/mozilla-central/diff/ab19291191fe/gfx/layers/basic/
> TextureHostX11.cpp#l1.20.

X11 uses a different byte order to most other drawing backends. The R and B channels are swapped. So you've got it right.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> The TextureHost implementation are basically identical, so it seems like a
> good idea to share that and just have a different TextureSource
> implementation.

Sweet!

> However it's not clear to me on how to decide when to use which

We can add a non-static GetBackendType method to Compositor, and let the X11 TextureHost decide which TextureSource to create based on that.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8376760 [details] [diff] [review]
wip

Review of attachment 8376760 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with a merge of the two X11 TextureHosts

Comment 7

[Tom S [:evilpie]]

Attachment: Introduce Compositor::GetBackendType

Comment 8

[Tom S [:evilpie]]

Attachment: Rename TextureHostX11 and split it

Comment 9

[Tom S [:evilpie]]

Attachment: Add new X11TextureSourceOGL

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8377242 [details] [diff] [review]
Add new X11TextureSourceOGL

Review of attachment 8377242 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/X11TextureSourceOGL.cpp
@@ +31,5 @@
> +  gl::sGLXLibrary.xBindTexImage(mSurface->XDisplay(), mSurface->GetGLXPixmap(),
> +                                LOCAL_GLX_FRONT_LEFT_EXT, NULL);
> +
> +  gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MIN_FILTER, LOCAL_GL_LINEAR);
> +  gl()->fTexParameteri(LOCAL_GL_TEXTURE_2D, LOCAL_GL_TEXTURE_MAG_FILTER, LOCAL_GL_LINEAR);

We should move this into getTemporaryTexture and only call it once per texture handle (A followup will do).

Comment 11

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57f2c6e6895e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd950a00e2cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eeffd689732

Thanks for the quick review!

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57f2c6e6895e
https://hg.mozilla.org/mozilla-central/rev/cd950a00e2cb
https://hg.mozilla.org/mozilla-central/rev/5eeffd689732