942302 - Use GLX pixmaps to provide faster compositing of WebGL with xrender

Status: RESOLVED DUPLICATE of bug 1572697

(Core :: Graphics: CanvasWebGL, enhancement, P3)

Description

[Kelsey Gilbert [:jgilbert]]

From talking with :karlt, it sounds like if we render into pixmaps for WebGL, we should get fast rendering when we hand them off to xrender. This should improve default performance of WebGL on linux where we don't have proper OGL accel. (the lack of which is the current default)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: wip

WIP, almost building.

I'm hitting a symbol resolution conflict, though:
 0:08.36 In file included from /home/jgilbert/ram/objs/obj-dbg/gfx/layers/Unified_cpp_gfx_layers1.cpp:9:
 0:08.36 In file included from /home/jgilbert/dev/mozilla/central/gfx/layers/client/ClientCanvasLayer.cpp:33:
 0:08.36 In file included from ../../dist/include/SharedSurfaceXRenderPixmap.h:9:
 0:08.36 In file included from ../../dist/include/gfxXlibSurface.h:11:
 0:08.36 In file included from ../../dist/system_wrappers/X11/extensions/Xrender.h:3:
 0:08.36 /usr/include/X11/extensions/Xrender.h:302:13: error: reference to 'Glyph' is ambiguous
 0:08.36                   _Xconst Glyph         *gids,
 0:08.36                           ^
 0:08.36 /usr/include/X11/extensions/render.h:31:14: note: candidate found by name lookup is 'Glyph'
 0:08.36 typedef XID             Glyph;
 0:08.36                         ^
 0:08.36 ../../dist/include/mozilla/gfx/2D.h:478:8: note: candidate found by name lookup is 'mozilla::gfx::Glyph'
 0:08.36 struct Glyph

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: webgl-pixmap

WFM. I'll post it to try and see how sad things are.

Comment 4

[Kelsey Gilbert [:jgilbert]]

https://tbpl.mozilla.org/?tree=Try&rev=7e585e18bbc6

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8343486 [details] [diff] [review]
webgl-pixmap

I started looking through this.  Some comments to date:

>+    virtual void SetGLXDrawableOverride(uint64_t drawable) {

XID is a typedef for long.  Making this parameter long will mean that no casts
are required.
 
>+    // Call with LOCAL_GLX_NONE to reset the override.

"None".  GLX_NONE is different.

>+        LOCAL_GLX_RENDER_TYPE, caps.alpha ? LOCAL_GLX_RGBA_BIT : 0,

GLX_RENDER_TYPE shouldn't depend on caps.alpha.
We probably want GLX_RGBA_BIT always.
GLX_COLOR_INDEX_BIT corresponds to GLX_PSEUDO_COLOR.  Usually 8-bit color.

Does LOCAL_GLX_DOUBLEBUFFER matter for offscreen contexts?

>+#include "nsAutoPtr.h"

template <class T> class nsRefPtr;

>+SharedSurface_XRenderPixmap::Fence()
>+{
>+    mGLX.xWaitGL();

>+    virtual bool WaitSync() MOZ_OVERRIDE { return true; }

I don't know exactly what Fence and WaitSync mean, but somewhere I assume we
need a glxWaitX() to ensure that reading from a surface has finished.

Also, note that glxWaitX is a no-op (but shouldn't be) with Mesa, and so
FinishX was used with tfp.
http://hg.mozilla.org/mozilla-central/annotate/551efcc4de6f/gfx/gl/GLContextProviderGLX.cpp#l446

Similarly, if there isn't already a glFlush with the Fence(), I suspect one will be needed with Mesa because glxWaitGL doesn't do the
right thing.
https://bugs.freedesktop.org/show_bug.cgi?id=52930

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Comment on attachment 8343486 [details] [diff] [review]
> webgl-pixmap
> 
> I started looking through this.  Some comments to date:
> 
> >+    virtual void SetGLXDrawableOverride(uint64_t drawable) {
> 
> XID is a typedef for long.  Making this parameter long will mean that no
> casts
> are required.
Fair.
>  
> >+    // Call with LOCAL_GLX_NONE to reset the override.
> 
> "None".  GLX_NONE is different.
Yep, I discovered this, but it looks like I forgot to update the comments.
> 
> >+        LOCAL_GLX_RENDER_TYPE, caps.alpha ? LOCAL_GLX_RGBA_BIT : 0,
> 
> GLX_RENDER_TYPE shouldn't depend on caps.alpha.
> We probably want GLX_RGBA_BIT always.
> GLX_COLOR_INDEX_BIT corresponds to GLX_PSEUDO_COLOR.  Usually 8-bit color.
This seems weird, but ok. I assume then that RGBA here is a misnomer, and it's actually RGB(X/A)?
> 
> Does LOCAL_GLX_DOUBLEBUFFER matter for offscreen contexts?
IIRC, pixmaps are single-buffered. 
> 
> >+#include "nsAutoPtr.h"
> 
> template <class T> class nsRefPtr;
> 
> >+SharedSurface_XRenderPixmap::Fence()
> >+{
> >+    mGLX.xWaitGL();
> 
> >+    virtual bool WaitSync() MOZ_OVERRIDE { return true; }
> 
> I don't know exactly what Fence and WaitSync mean, but somewhere I assume we
> need a glxWaitX() to ensure that reading from a surface has finished.
We don't actually fence on both sides of the operation, currently. I think that glxWaitGL should actually be in WaitSync instead of Fence, anyways.
Fence is called when Content is done rendering a frame. WaitSync is called by the consumer (compositor) when it wants to make sure the frame data is actually realized.
> 
> Also, note that glxWaitX is a no-op (but shouldn't be) with Mesa, and so
> FinishX was used with tfp.
> http://hg.mozilla.org/mozilla-central/annotate/551efcc4de6f/gfx/gl/
> GLContextProviderGLX.cpp#l446
> 
> Similarly, if there isn't already a glFlush with the Fence(), I suspect one
> will be needed with Mesa because glxWaitGL doesn't do the
> right thing.
> https://bugs.freedesktop.org/show_bug.cgi?id=52930
Gross, but ok.

Comment 7

[Nick Cameron [:nrc]]

Comment on attachment 8343486 [details] [diff] [review]
webgl-pixmap

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

Just checked the layers stuff. The GLContext and X things I don't know well enough to review. Not sure if karl will cover GLContext, might be good to ask bjacob to review that.

::: gfx/layers/CopyableCanvasLayer.cpp
@@ +63,5 @@
> +    // Possibly morph to a faster SurfaceFactory type.
> +    GLScreenBuffer* screen = mGLContext->Screen();
> +    const SurfaceCaps& caps = screen->Caps();
> +    SurfaceFactory_GL* factory = nullptr;
> +    if (!mForceReadback) {

nit: merge with below if

@@ +72,5 @@
> +#endif
> +    }
> +
> +    if (factory) {
> +      SurfaceStreamType streamType = 

whitespace

@@ +130,5 @@
>      if (!sharedSurf) {
>        NS_WARNING("Null frame received.");
>        return;
>      }
> +    if (!sharedSurf->HasAlpha()) {

merge the if clause into the assert

@@ +133,5 @@
>      }
> +    if (!sharedSurf->HasAlpha()) {
> +      MOZ_ASSERT(GetContentFlags() & CONTENT_OPAQUE);
> +    }
> +    

whitespace

@@ +159,4 @@
>      } else {
> +      // Non-ShSurfBasic readback. Not a standard GLContext path.
> +      // Don't bother to optimize away copies for this.
> +      MOZ_ASSERT(!tempSurf);

seems unnecessary

@@ +161,5 @@
> +      // Don't bother to optimize away copies for this.
> +      MOZ_ASSERT(!tempSurf);
> +      tempSurf = GetTempSurface(readSize, format);
> +      // Readback handles Flush/MarkDirty.
> +      mGLContext->Screen()->Readback(surfGL, tempSurf);

does this always succeed?

@@ +167,5 @@
> +
> +      if (!destSurf) {
> +        destSurf = srcSurf; // Avoid creating multiple temp surfs
> +        // Also, since this is a temp surf, we can modify it, unlike the data
> +        // in srcSurf, which we can't safely change.

this comment is unclear to me, aren't we just aliasing srcSurf/tempSurf, so the data is the same?

@@ +193,5 @@
> +        tempSurf = GetTempSurface(readSize, format);
> +        destSurf = tempSurf;
> +      }
> +      MOZ_ASSERT(destSurf);
> +      

whitespace

@@ +194,5 @@
> +        destSurf = tempSurf;
> +      }
> +      MOZ_ASSERT(destSurf);
> +      
> +      nsRefPtr<gfxImageSurface> mutableSurf = destSurf->GetAsImageSurface(); // Nullable

nit don't need the comment - it is obvious from the next if statement

@@ +223,3 @@
>      }
> +
> +    if (!destSurf)

{}, but I don't think we ever hit this - you assert destSurf above and don't null it.

@@ +224,5 @@
> +
> +    if (!destSurf)
> +      destSurf = srcSurf;
> +
> +    if (srcSurf != destSurf)

{}

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +46,5 @@
>   
>    mCanvasClient = nullptr;
>  
>    if (mGLContext) {
> +		mNeedsYFlip = true;

indent

@@ +47,5 @@
>    mCanvasClient = nullptr;
>  
>    if (mGLContext) {
> +		mNeedsYFlip = true;
> +		

whitespace

@@ +53,4 @@
>      SurfaceFactory_GL* factory = nullptr;
>      if (!mForceReadback) {
> +      const SurfaceCaps& caps = screen->Caps();
> +      

whitespace

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8343486 [details] [diff] [review]
webgl-pixmap

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> (In reply to Karl Tomlinson (:karlt) from comment #5)
> > 
> > >+        LOCAL_GLX_RENDER_TYPE, caps.alpha ? LOCAL_GLX_RGBA_BIT : 0,
> > 
> > GLX_RENDER_TYPE shouldn't depend on caps.alpha.
> > We probably want GLX_RGBA_BIT always.
> > GLX_COLOR_INDEX_BIT corresponds to GLX_PSEUDO_COLOR.  Usually 8-bit color.
> This seems weird, but ok. I assume then that RGBA here is a misnomer, and
> it's actually RGB(X/A)?

Yes, I assume RGBA just means there are separate channels rather than indices, even if some of the channels may have zero bits.

> > 
> > Does LOCAL_GLX_DOUBLEBUFFER matter for offscreen contexts?
> IIRC, pixmaps are single-buffered. 

http://www.opengl.org/sdk/docs/man2/xhtml/glXCreateGLXPixmap.xml says "GLX
pixmaps with double-buffering are supported.  However, glXSwapBuffers is
ignored by these pixmaps."
I guess that means that pixmaps with double buffering behave like those
without, so GLX_DOUBLEBUFFER doesn't matter here.

> > >+SharedSurface_XRenderPixmap::Fence()
> > >+{
> > >+    mGLX.xWaitGL();
> > 
> > >+    virtual bool WaitSync() MOZ_OVERRIDE { return true; }
> > 
> > I don't know exactly what Fence and WaitSync mean, but somewhere I assume we
> > need a glxWaitX() to ensure that reading from a surface has finished.
> We don't actually fence on both sides of the operation, currently.

That may not be too bad with the way layers are always only reused for the
same content.  If reading has not finished when the new bits are being drawn,
then the reading will read new bits instead of the old.  That may produce some
flickering or tearing but eventually all new bits should be displayed.

> I think
> that glxWaitGL should actually be in WaitSync instead of Fence, anyways.
> Fence is called when Content is done rendering a frame. WaitSync is called
> by the consumer (compositor) when it wants to make sure the frame data is
> actually realized.

I guess the best implementation here would set a flag in Fence, and
glxWaitGL/Flush in WaitSync if the flag has been set.

>-    GLXPixmap glxpixmap = 0;

>+    GLXPixmap pixmap;

I found this less confusing when the variable was named |glxpixmap|.

>+    MOZ_ASSERT(!caps.any);
>+    SurfaceCaps offscreenCaps = caps;
>+    if (caps.antialias) {
>+        offscreenCaps.depth = false;
>+        offscreenCaps.stencil = false;
>+        offscreenCaps.antialias = false;
>+    }

I don't know why antialias implies depth and stencil are false.

>+/*
>+#ifdef MOZ_WIDGET_GTK
>+#include <gdk/gdk.h>
>+#include <gdk/gdkx.h>
>+#define GET_NATIVE_WINDOW(aWidget) GDK_WINDOW_XID((GdkWindow *) aWidget->GetNativeData(NS_NATIVE_WINDOW))
>+#elif defined(MOZ_WIDGET_QT)
>+#include <QWidget>
>+#define GET_NATIVE_WINDOW(aWidget) static_cast<QWidget*>(aWidget->GetNativeData(NS_NATIVE_SHELLWIDGET))->winId()
>+#endif
>+*/

This can be removed, I assume.

>+                                 GLXLibrary& glx,

>+    GLXLibrary& mGLX;

Convention is usually to use a pointer, isn't it?
What was the motivation for using a reference?

>-                            ? gfxImageFormatRGB24
>-                            : gfxImageFormatARGB32;

>+                          ? gfxImageFormatRGB24
>+                          : gfxImageFormatARGB32;

Unnecessary whitespace change.

r+ on the gl/ changes, assuming the build can be fixed as well as the
X_PutImage BadDrawable and ASAN crash.  Any ideas there?

I quickly looked over the layers changes, but didn't review them.
I'll leave those to nrc.

Comment 10

[madbiologist]

Any progress to report?

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to madbiologist from comment #10)
> Any progress to report?

This is pretty heavily bitrotted. IIRC, the issue I was having that prevented landing was a number of miscellaneous GLX errors when run on Try.

It's worth picking this up again at some point, as the core concept is still valid. Not right now though.

Comment 12

[Robert Mader [:rmader]]

I guess this can now be closed, right? We now use DMABuf sharing instead.