Closed Bug 640082 Opened 9 years ago Closed 9 years ago

Use texture_from_pixmap on GLX when possible

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(7 files, 15 obsolete files)

4.12 KB, patch
joe
: review+
Details | Diff | Splinter Review
4.74 KB, patch
joe
: review+
Details | Diff | Splinter Review
8.49 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.51 KB, patch
joe
: review+
Details | Diff | Splinter Review
8.23 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
805 bytes, patch
bjacob
: review+
Details | Diff | Splinter Review
2.37 KB, patch
Details | Diff | Splinter Review
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.
Attached patch Prototype Canvas Impl (obsolete) — Splinter Review
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.
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.
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.
Attachment #517975 - Attachment is obsolete: true
Attachment #517999 - Attachment is obsolete: true
Attachment #518000 - Attachment is obsolete: true
(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.
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?
Attachment #518214 - Attachment is obsolete: true
Attachment #520571 - Attachment is patch: true
Attachment #520571 - Attachment mime type: application/octet-stream → text/plain
Attachment #520572 - Attachment description: Part 4: Use texture_from_pixmap in CairoImageOGL → Part 5: Use texture_from_pixmap in CairoImageOGL
(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.
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!
Attachment #518213 - Attachment is obsolete: true
Attachment #520577 - Flags: review?(joe)
Attachment #520570 - Attachment is obsolete: true
Attachment #520578 - Flags: review?(joe)
Attachment #518215 - Attachment is obsolete: true
Attachment #520579 - Flags: review?(joe)
Attachment #520571 - Attachment is obsolete: true
Attachment #520580 - Flags: review?(joe)
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 :)
Attachment #520572 - Attachment is obsolete: true
Attachment #520581 - Flags: review?(joe)
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.
Fixed a few bugs found during running reftests.
Attachment #520578 - Attachment is obsolete: true
Attachment #520578 - Flags: review?(joe)
Attachment #522835 - Flags: review?(joe)
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.
Attachment #520580 - Attachment is obsolete: true
Attachment #520580 - Flags: review?(joe)
Attachment #522836 - Flags: review?(joe)
Assignee: nobody → matt.woodrow+bugzilla
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.
Attachment #520577 - Flags: review?(joe) → review+
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".
Attachment #520579 - Flags: review?(joe) → review+
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.
Attachment #522836 - Flags: review?(joe) → review+
Attachment #522835 - Flags: review?(joe) → review+
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.
Attachment #520581 - Flags: review?(joe) → review+
(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.
(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 :)
Fixed review comments.

Carrying forward r=joe
Attachment #520577 - Attachment is obsolete: true
Attachment #523153 - Flags: review+
Added ScopedBindTexture.

Asking for another review since this is a big change to the patch.
Attachment #520579 - Attachment is obsolete: true
Attachment #523155 - Flags: review?(joe)
Fixed review comments.

Carrying forward r=joe
Attachment #522836 - Attachment is obsolete: true
Attachment #523157 - Flags: review+
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!
Attachment #523155 - Flags: review?(joe) → review+
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?
We worked on this together with Matt and he wrote this patch, so I'm taking the liberty of r+ing myself.
Attachment #525195 - Flags: review+
This should fix the reported compilation issue.
Attachment #525196 - Flags: review?(matt.woodrow+bugzilla)
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_
(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!
Duplicate of this bug: 648741
Duplicate of this bug: 575505
Attachment #525196 - Flags: review?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.