640082 - Use texture_from_pixmap on GLX when possible

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Matt Woodrow (:mattwoodrow)]

We should use the texture_from_pixmap extension to avoid needing to read data back from the X server.

http://www.opengl.org/registry/specs/EXT/texture_from_pixmap.txt

A GLXPixmap can only be created once for each X surface. I'm not sure how this would work when you share a surface between processes, I don't see any way to check if one exists, or to retrieve it.

One option would be to create a gfxXlibSurface subclass that includes a GLXPixmap and hook creation of these into CreateOptimalSurface. We could also just let each layer type handle things in their own way.

ImageLayer looks like it could be a problem, the data needs to be copied during SetData(), we can't rely on it still being available during Render(). In the plugin-sanity reftest, we get the exact same X surface being passed for each of the four CairoImage objects.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Prototype Canvas Impl

WIP implementation for CanvasLayers.

This takes my fps on the 'Psychedelic Browsing' benchmark from 10 -> 1000 (and  423 on the second part) which is almost identical to the results without accelerated layers. Nice to see we're no longer hindering XRender, and can look to improve on more comples test cases.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Sidenote that I'm sure would get commented on otherwise: We need to make this code GLX only, and check for the existence of the extension rather than relying on it.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Add texture_from_pixmap support to the context provider.

I figure it's easier to just add these to GLXLibrary and #ifdef MOZ_X11 in places where we use it rather than create more abstract interfaces on GLContext that will only ever be used by this.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Use texture from pixmap in CanvasLayer

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add texture_from pixmap support to GLContextProviderGLX

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Use texture_from_pixmap in CanvasLayerOGL

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add BindTexture functions to TextureImage

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create TextureImageGLX which uses texture_from_pixmap

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #0)
> ImageLayer looks like it could be a problem, the data needs to be copied during
> SetData(), we can't rely on it still being available during Render().

Hmm, nobody should be modifying the X surface while the image is the current image.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Use texture_from_pixmap in CanvasLayerOGL v2

Added XGrabServer calls to prevent flickering.

I really don't understand why these are necessary, we already call XSync and the next stage of rendering to the pixmap shouldn't happen until after swapbuffers/flush. Nothing else should touch the pixmap during this period?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create TextureImageGLX which uses texture_from_pixmap v2

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use texture_from_pixmap in CairoImageOGL

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to comment #10)
> Added XGrabServer calls to prevent flickering.
> 
> I really don't understand why these are necessary, we already call XSync and

glXWaitX should be nicer than XSync in some situations.

> the next stage of rendering to the pixmap shouldn't happen until after
> swapbuffers/flush. Nothing else should touch the pixmap during this period?

I don't know where the swapbuffers/flush happens, but a glXWaitGL would be gentler than a GrabServer.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add texture_from pixmap support to GLContextProviderGLX v2

Added glXWaitGL after swapping buffers. SwapBuffers() appears to only glFlush, not glFinish.

Moved XSync into the BindPixmap function. Interesting glXWaitX is considerably slower than XSync, contrary to what the 'docs' would have you believe. glXWaitX does skip a trip to the server and could be useful if we ever have a remote X server.

Though for this case I think we'd also want to ask all layers to do their X drawing first, issue a single flush call, and then composite the layers.

Thanks for the suggestion Karl!

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Use texture_from_pixmap in CanvasLayerOGL v3

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add BindTexture functions to TextureImage

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create TextureImageGLX which uses texture_from_pixmap v3

Comment 18

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Use texture_from_pixmap in CairoImageOGL

We create a new GLXPixmap and then delete it straight away because the X pixmap can be set on multiple live images at a time, and attempting to create multiple GLXPixmaps for a single Pixmap is an error.

The 'better' solution here would be create a way to tie a GLXPixmap to an X pixmap so it can be looked up later. gfxXlibSurfaceThatWeAlsoCreatedAGLXPixmapFor?

I feel it's probably easier to just do it this way, and look into changing it later if it shows up in profiles.

NB: Using joe as my default-gl-reviewer here, I'm happy for anyone else to steal these :)

Comment 19

[donrhummy]

How do I get a tryserver build? I've filed this issue ( https://bugzilla.mozilla.org/show_bug.cgi?id=642694 ) and wanted to help you guys figure out if the patches fix the problem.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Tryserver builds should be available from http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-845c1f0bce87 soon.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Use texture_from_pixmap in CanvasLayerOGL v4

Fixed a few bugs found during running reftests.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create TextureImageGLX which uses texture_from_pixmap v4

Clear the new X surface before using it.

With these patches (and a patch for force-enable accelerated layers), I get green reftest runs on tryserver.

Note that we won't get component alpha on linux with this until we add a path for that into SurfaceBufferOGL. I don't really know how to handle this yet, will think about it.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 520577 [details] [diff] [review]
Part 1: Add texture_from pixmap support to GLContextProviderGLX v2

Overall: looks great - some small comments:


>+GLXPixmap 
>+GLXLibrary::CreatePixmap(gfxASurface* aSurface)

>+    nsAutoTArray<int, 20> attribs;
>+
>+#define A1_(_x)  do { attribs.AppendElement(_x); } while(0)
>+#define A2_(_x,_y)  do {                                                \
>+        attribs.AppendElement(_x);                                      \
>+        attribs.AppendElement(_y);                                      \
>+    } while(0)

These could be called APPEND1 and APPEND2, and should be #undefed at the end of the function (or after you're done using them).

However - is there any reason you don't just use a statically-defined array, like you do for the pixmapAttribs below?

>+    A2_(GLX_DOUBLEBUFFER, False);
>+    A2_(GLX_DRAWABLE_TYPE, GLX_PIXMAP_BIT);
>+    A2_(GLX_BIND_TO_TEXTURE_RGBA_EXT, True);
>+    A1_(0);

This 0 should be None instead, just because it's an X call.

>+    if (!cfg) {
>+        return nsnull;

I think this should be 0 instead of nsnull, since GLXPixmap is just an int, right?

>+    }
>+    NS_ASSERTION(numFormats > 0,
>+                 "glXChooseFBConfig() failed to match our requested format and violated its spec (!)");

Does this really violate the spec?

Also, maybe we should NS_ABORT_IF_FALSE, so we really notice :)

>+
>+    gfxXlibSurface *xs = static_cast<gfxXlibSurface*>(aSurface);
>+
>+    int pixmapAttribs[] = { GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT,
>+                            GLX_TEXTURE_FORMAT_EXT, GLX_TEXTURE_FORMAT_RGBA_EXT,
>+                            0};

0 should be replaced with None.

>+GLXLibrary::BindTexImage(GLXPixmap aPixmap)

>+    Display *display = DefaultXDisplay();
>+    XSync(DefaultXDisplay(), False);

You should put a comment above this XSync noting that it exists to ensure that we're going to have the complete results of X rendering in the bound texture.

Comment 24

[Joe Drew (not getting mail)]

Comment on attachment 520579 [details] [diff] [review]
Part 3: Add BindTexture functions to TextureImage

What do you think of making a ScopedBindTexture so we can't screw things up accidentally?

>diff --git a/gfx/thebes/GLContext.h b/gfx/thebes/GLContext.h

>+    virtual void BindTexture() =0;

Add a space before "0".

Comment 25

[Joe Drew (not getting mail)]

Comment on attachment 522836 [details] [diff] [review]
Part 4: Create TextureImageGLX which uses texture_from_pixmap v4


>diff --git a/gfx/thebes/GLContextProviderGLX.cpp b/gfx/thebes/GLContextProviderGLX.cpp

>+    PRBool TextureImageSupportsGetBackingSurface()
>+    {
>+        if (!sGLXLibrary.HasTextureFromPixmap()) {
>+            return PR_FALSE;
>+        }
>+        return PR_TRUE;
>+    }

You can just return sGLXLibrary.HasTextureFromPixmap() here I think.

>+    virtual already_AddRefed<TextureImage>
>+    CreateTextureImage(const nsIntSize& aSize,
>+                       TextureImage::ContentType aContentType,
>+                       GLenum aWrapMode,
>+                       PRBool aUseNearestFilter=PR_FALSE);

Can you add spaces on either side of "=" here?

>+class TextureImageGLX : public TextureImage

>+    virtual PRBool InUpdate() const { return !!mUpdateSurface; }

Won't this always return true, since mUpdateSurface is never nulled out? Maybe that doesn't matter.

>+GLContextGLX::CreateTextureImage(const nsIntSize& aSize,
>+                                 TextureImage::ContentType aContentType,
>+                                 GLenum aWrapMode,
>+                                 PRBool aUseNearestFilter)
>+{

>+    MakeCurrent();
>+    GLXPixmap pixmap = sGLXLibrary.CreatePixmap(surface);
>+    NS_ASSERTION(pixmap, "Failed to create pixmap!");
>+
>+    GLuint texture;
>+    fGenTextures(1, &texture);
>+
>+    fActiveTexture(LOCAL_GL_TEXTURE0);
>+    fBindTexture(LOCAL_GL_TEXTURE_2D, texture);

It's sort of ugly that we have to do this texture generation and tex parameter stuff in this function instead of in the TextureImageGLX constructor, since doing it in the constructor this would make it so that fGenTextures and fDeleteTextures calls would be in the same location. Even the CreatePixmap call could be inside the constructor, in fact.

Maybe we should file a bug on making TextureImage manage its texture's creation _and_ deletion, if that's possible.

Comment 26

[Joe Drew (not getting mail)]

Comment on attachment 520581 [details] [diff] [review]
Part 5: Use texture_from_pixmap in CairoImageOGL

Overall I'm not super-enthusiastic about this patch, and I think the reason I feel that way is because it's sort of a surgical insertion of the GLX code rather than integrating it more fundamentally. I don't have as big of an issue with CanvasLayerOGL because it's less intrusive, I think.

I'm not sure if there's a better solution aside from changing CairoImageOGL to use TextureImage, which isn't gonna happen anytime soon.

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #23)
>
> 
> Does this really violate the spec?
> 
> Also, maybe we should NS_ABORT_IF_FALSE, so we really notice :)
> 

Very much so. The spec says that it will either return NULL and 0, or a pointer and how many objects are being returned. If it returns a pointer, but says there are 0 found, we don't know what to do. Changed to NS_ABORT_IF_FALSE.

Comment 28

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #24)
> Comment on attachment 520579 [details] [diff] [review]
> Part 3: Add BindTexture functions to TextureImage
> 
> What do you think of making a ScopedBindTexture so we can't screw things up
> accidentally?
> 

Only annoyance at not thinking of this first :)

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add texture_from pixmap support to GLContextProviderGLX v3

Fixed review comments.

Carrying forward r=joe

Comment 30

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add BindTexture functions to TextureImage v2

Added ScopedBindTexture.

Asking for another review since this is a big change to the patch.

Comment 31

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Create TextureImageGLX which uses texture_from_pixmap v5

Fixed review comments.

Carrying forward r=joe

Comment 32

[Joe Drew (not getting mail)]

Comment on attachment 523155 [details] [diff] [review]
Part 3: Add BindTexture functions to TextureImage v2

Put a comment above the definition of ScopedBindTexture and you win!

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cd13b3b102e3
http://hg.mozilla.org/mozilla-central/rev/bab34375319d
http://hg.mozilla.org/mozilla-central/rev/ea1b21a2bf82
http://hg.mozilla.org/mozilla-central/rev/6f831c7a1990
http://hg.mozilla.org/mozilla-central/rev/24a6d10dd3a2

Comment 34

[Bartłomiej Brzozowiec (BartZilla)]

I can no longer compile trunk on my system due to changes from this bug:

GLContextProviderGLX.cpp
(...)
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp: In member function 'GLXPixmap mozilla::gl::GLXLibrary::CreatePixmap(gfxASurface*)':
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:301: error: 'GLX_BIND_TO_TEXTURE_RGBA_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:320: error: 'GLX_TEXTURE_TARGET_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:320: error: 'GLX_TEXTURE_2D_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:321: error: 'GLX_TEXTURE_FORMAT_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:321: error: 'GLX_TEXTURE_FORMAT_RGBA_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp: In member function 'void mozilla::gl::GLXLibrary::BindTexImage(GLXPixmap)':
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:353: error: 'GLX_FRONT_LEFT_EXT' was not declared in this scope
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp: In member function 'void mozilla::gl::GLXLibrary::ReleaseTexImage(GLXPixmap)':
/home/firefox/src/gfx/thebes/GLContextProviderGLX.cpp:364: error: 'GLX_FRONT_LEFT_EXT' was not declared in this scope

The system is Linux. Sure, some libs here are a little bit rusty, but it compiled before. What can I do to fix this? Should I fill new bug about it?

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: GL_TEXTURE -> GL_TEXTURE0 fix

We worked on this together with Matt and he wrote this patch, so I'm taking the liberty of r+ing myself.

Comment 36

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: add missing GLX_..._EXT defines

This should fix the reported compilation issue.

Comment 37

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 525196 [details] [diff] [review]
add missing GLX_..._EXT defines

Won't having the same defines as the glxext.h header cause conflicts if both don't have GLX_EXT_texture_from_pixmap defined?

It might be easier to remove the #ifndef, and prefix all of these (and our usage of them) with LOCAL_

Comment 38

[Joe Drew (not getting mail)]

Landed attachment 525195 [details] [diff] [review]: http://hg.mozilla.org/mozilla-central/rev/48d6abe0a436

Comment 39

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #36)
> Created attachment 525196 [details] [diff] [review]
> add missing GLX_..._EXT defines
> 
> This should fix the reported compilation issue.

Yes, it works. Thanks!