Use texture from pixmap with X11 and OpenGL

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla30
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Posted patch texturehost-ogl (obsolete) — Splinter Review
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.
Posted patch wip (obsolete) — Splinter Review
I fixed the color problem and cleaned up some stuff.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8376760 - Flags: feedback?(nical.bugzilla)
Attachment #8376760 - Attachment is patch: true
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?
(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.
(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.
(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 on attachment 8376760 [details] [diff] [review]
wip

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

f+ with a merge of the two X11 TextureHosts
Attachment #8376760 - Flags: feedback?(nical.bugzilla) → feedback+
Attachment #8376742 - Attachment is obsolete: true
Attachment #8376760 - Attachment is obsolete: true
Attachment #8377240 - Flags: review?(nical.bugzilla)
Attachment #8377241 - Flags: review?(nical.bugzilla)
Attachment #8377242 - Flags: review?(nical.bugzilla)
Attachment #8377242 - Attachment is patch: true
Attachment #8377240 - Flags: review?(nical.bugzilla) → review+
Attachment #8377241 - Flags: review?(nical.bugzilla) → review+
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).
Attachment #8377242 - Flags: review?(nical.bugzilla) → review+
Depends on: 974709
Blocks: 976015
Depends on: 977963
Depends on: 1000542
No longer depends on: 1000542
You need to log in before you can comment on or make changes to this bug.