571832 - GL ThebesLayer rendering layout on X-System terribly slow.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

ThebesLayerOGL::RenderLayer, and probably some other render layer implementations are very slow in case of rendering on X linux system.

Problem is that we are rendering layout to X-Surface (coming from CreateOffscreenSurface http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#161), and then copy data from X-Surface to ImageSurface (XGetImage!!  http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#189)

And then uploading image surface to GPU, which is also slow:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#206


I think we should must fix at least for X
And render X-Surface directly to the screen.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: WIP1, Rendering to XlibSurface and fast texture binding to XSurface

Comment 2

[Oleg Romashin (:romaxa)]

Comment on attachment 451116 [details] [diff] [review]
WIP1, Rendering to XlibSurface and fast texture binding to XSurface

With this patch, rendering works almost just fine...
I see one problem here, is that ThebesLayer not cached... 
In logs I see ctor/paint/dtor.... is it expected behavior?

Comment 3

[Bas Schouten (:bas.schouten)]

(In reply to comment #2)
> (From update of attachment 451116 [details] [diff] [review])
> With this patch, rendering works almost just fine...
> I see one problem here, is that ThebesLayer not cached... 
> In logs I see ctor/paint/dtor.... is it expected behavior?

As long as retained layers aren't used, yes.

Comment 4

[Oleg Romashin (:romaxa)]

Attachment: WIP2, Seems to work

Added GLSurface class (Bind/Release, init/destroy)
Added KHR extension handling,
Fixed layer rendering
Some cleanup.

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: WIP3, moved native Image creation to GLContext

Minor issue with glEGLImageTargetTexture2DOES binding.. but feedback about structure and API is needed

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: CreateFromASurface Provide API,

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Switch ThebesLayer to new API

Comment 8

[Bas Schouten (:bas.schouten)]

I will try to have this reviewed tomorrow (20100617).

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Fixed ThebesLayer use Bind/Release APi (removed multiple binding)

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: CanvasLayerOGL use bind/release TexImage API

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: ImageLayerOGL use new API

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 451700 [details] [diff] [review]
CreateFromASurface Provide API, 

># HG changeset patch
># Parent 2c4b995df5a8a651815e629761dd7ae3dce26d2f
>
>diff -r 2c4b995df5a8 gfx/thebes/public/GLContext.h
>--- a/gfx/thebes/public/GLContext.h	Wed Jun 16 20:17:36 2010 +0300
>+++ b/gfx/thebes/public/GLContext.h	Wed Jun 16 23:56:07 2010 +0300
>@@ -153,16 +153,18 @@ public:
>      * keping the same format and attributes.  If the resize succeeds, return
>      * PR_TRUE.  Otherwise, or if this is not a pbuffer, return PR_FALSE.
>      *
>      * On a successful resize, the previous contents of the pbuffer are cleared,
>      * and the new contents are undefined.
>      */
>     virtual PRBool Resize(const gfxIntSize& aNewSize) { return PR_FALSE; }
> 
>+    virtual PRBool BindTexImage() { return PR_FALSE; };
>+    virtual PRBool ReleaseTexImage() { return PR_FALSE; };

^ bogus semicolons at the end of functions

>diff -r 2c4b995df5a8 gfx/thebes/public/GLContextProvider.h
>--- a/gfx/thebes/public/GLContextProvider.h	Wed Jun 16 20:17:36 2010 +0300
>+++ b/gfx/thebes/public/GLContextProvider.h	Wed Jun 16 23:56:07 2010 +0300
>@@ -38,16 +38,17 @@
> #define GLCONTEXTPROVIDER_H_
> 
> #include "GLContext.h"
> #include "gfxTypes.h"
> #include "gfxPoint.h"
> #include "nsAutoPtr.h"
> 
> class nsIWidget;
>+class gfxASurface;
> 
> namespace mozilla {
> namespace gl {
> 
> class THEBES_API GLContextProvider 
> {
> public:
>     struct ContextFormat {
>@@ -114,16 +115,18 @@ public:
>     /**
>      * Create a context that renders to the surface of the widget that is
>      * passed in.
>      *
>      * @param Widget whose surface to create a context for
>      * @return Context to use for this window
>      */
>     already_AddRefed<GLContext> CreateForWindow(nsIWidget *aWidget);
>+
>+    already_AddRefed<GLContext> CreateForASurface(gfxASurface *aSurface);

^ This needs docs (and probably refreshing?) -- not sure how you create a GLContext for an arbitrary gfxASurface.


>diff -r 2c4b995df5a8 gfx/thebes/src/GLContextProviderEGL.cpp
>--- a/gfx/thebes/src/GLContextProviderEGL.cpp	Wed Jun 16 20:17:36 2010 +0300
>+++ b/gfx/thebes/src/GLContextProviderEGL.cpp	Wed Jun 16 23:56:07 2010 +0300

>+    typedef void (*pfnImageTargetTexture2DOES)(GLenum target, GLeglImageOES image);
>+    pfnImageTargetTexture2DOES fImageTargetTexture2DOES;

This is a GL symbol, not an EGL symbol; I guess it's ok to stick it on here, though that should be mentioned in a comment or something.

>+            // this isn't an error, just means that KHR aren't supported

"just means that EGL_image_* isn't supported"

> class GLContextEGL : public GLContext
> {
> public:
>     GLContextEGL(EGLDisplay aDisplay, EGLConfig aConfig, 
>-                 EGLSurface aSurface, EGLContext aContext)
>+                 EGLSurface aSurface, EGLContext aContext,
>+                 EGLImageKHR aImage = nsnull,
>+                 gfxASurface *aASurface = nsnull)
>         : mDisplay(aDisplay), mConfig(aConfig), 
>           mSurface(aSurface), mContext(aContext)
>+        , mImage(aImage), mASurface(aASurface)
>+        , mBinded(PR_FALSE)

"mBound", but see below.

>+    PRBool BindTexImage()
>+    {
>+        if (mBinded)
>+            return PR_TRUE;

You can't just check a local variable here and ignore the binding -- the same EGLImage might have been bound elsewhere, but is now being bound to a different texture, different texture unit, etc.  I think the only safe thing you can is check if it's bound, and if it is, then call ReleaseTexImage before redoing the binding.

>+        if (!mDisplay)
>+            return PR_FALSE;

Not sure if you need to check for this -- don't think it can ever happen.

>+        if (mSurface) {
>+            EGLBoolean success;
>+            success = sEGLLibrary.fBindTexImage(mDisplay, (EGLSurface)mSurface,
>+                                                LOCAL_EGL_BACK_BUFFER);
>+            if (success == LOCAL_EGL_FALSE)
>+                return PR_FALSE;

^ Ok, so this takes care of binding a pbuffer surface to the current texture

>+        } else if (mImage) {
>+            sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, mImage);

^ And this takes care of binding an EGLImage.  I guess that's ok, though they're somewhat different operations, but ultimately end up with the same behaviour.

>+already_AddRefed<GLContext>
>+GLContextProvider::CreateForASurface(gfxASurface *aSurface)

This whole function assumes MOZ_X11; it should at least have those ifdefs around the implementation.

>+{
>+    EGLDisplay display = nsnull;
>+    EGLSurface surface = nsnull;
>+    EGLImageKHR image = nsnull;
>+
>+    if (!sEGLLibrary.EnsureInitialized())
>+        return nsnull;
>+
>+    if (aSurface->GetType() != gfxASurface::SurfaceTypeXlib)
>+        return nsnull;
>+
>+    gfxXlibSurface *xsurface = static_cast<gfxXlibSurface*>(aSurface);
>+
>+    display = sEGLLibrary.fGetDisplay((EGLNativeDisplayType)xsurface->XDisplay());
>+    if (!display)
>+        return nsnull;
>+
>+    if (!sEGLLibrary.fInitialize(display, NULL, NULL))
>+        return nsnull;

>+    if (sEGLLibrary.fCreateImageKHR && !getenv("NO_KHR_EXT")) {

"NO_KHR_EXT" is not a good env var if you want to disable this; call it "NO_EGL_IMAGE" or something.

>+already_AddRefed<GLContext>
>+GLContextProvider::CreateForASurface(gfxASurface *aSurface)
>+{
>+    return nsnull;
>+}

It occurs to me that this should probably be "CreateForNativePixmapSurface", because in general that's the only type of thing we'll be able to create an EGLImage from.  Calling it ASurface just implies some generic-ness that's not really true.

> } /* namespace gl */

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 452804 [details] [diff] [review]
Fixed ThebesLayer use Bind/Release APi (removed multiple binding)


>+PRBool
>+ThebesLayerOGL::EnsureSurface()
>+{
>+  gfxASurface::gfxImageFormat imageFormat = gfxASurface::ImageFormatARGB32;
>+  if (UseOpaqueSurface(this))
>+    imageFormat = gfxASurface::ImageFormatRGB24;
>+
>+  if (mInvalidatedRect.IsEmpty())
>+    return mOffScreenSurface ? PR_TRUE : PR_FALSE;
>+
>+  if ((mOffscreenSize == gfxIntSize(mInvalidatedRect.width, mInvalidatedRect.height)
>+      && imageFormat == mOffscreenFormat)
>+      || mInvalidatedRect.IsEmpty())
>+    return mOffScreenSurface ? PR_TRUE : PR_FALSE;
>+
>+  mOffScreenSurface =
>+    gfxPlatform::GetPlatform()->
>+      CreateOffscreenSurface(gfxIntSize(mInvalidatedRect.width, mInvalidatedRect.height),
>+                             imageFormat);
>+  if (!mOffScreenSurface)
>+    return PR_FALSE;
>+
>+  if (mOffScreenSurface) {
>+    mOffscreenSize.width = mInvalidatedRect.width;
>+    mOffscreenSize.height = mInvalidatedRect.height;
>+    mOffscreenFormat = imageFormat;
>+  }
>+  mGLSurface = sGLContextProvider.CreateForASurface(mOffScreenSurface);
>+  if (!mGLSurface)
>+    return PR_TRUE;
>+
>+  return PR_TRUE;

^^ ??  The first check/return should probably removed, and a comment be added saying that it's ok for the mGLSurface creation to fail.  Also, "mGLSurface" is a bad name -- it's a full GLContext, so should be called something else.

>-  gl()->fTexImage2D(LOCAL_GL_TEXTURE_2D,
>-                    0,
>-                    LOCAL_GL_RGBA,
>-                    mInvalidatedRect.width,
>-                    mInvalidatedRect.height,
>-                    0,
>-                    LOCAL_GL_RGBA,
>-                    LOCAL_GL_UNSIGNED_BYTE,
>-                    NULL);
>-

Why get rid of this here..

>     gl()->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture);
>+    gl()->fTexImage2D(LOCAL_GL_TEXTURE_2D,
>+                      0,
>+                      LOCAL_GL_RGBA,
>+                      mInvalidatedRect.width,
>+                      mInvalidatedRect.height,
>+                      0,
>+                      LOCAL_GL_RGBA,
>+                      LOCAL_GL_UNSIGNED_BYTE,
>+                      NULL);

To move it down here?  I guess because Bind will mean that we already have texture storage.  Still, this isn't good here, there's no point in calling TexImage2D with NULL and then TexSubImage2D right after.  And TexImage2D with NULL doesn't make any guarantees about the contents, so they could be junk -- you either ave to call TexImage2D and make sure you render to the whole thing, or call TexSubImage and make sure that the rest of the texture is already valid before filling in the changed bit.

>     gl()->fTexSubImage2D(LOCAL_GL_TEXTURE_2D,
>                          0,
>                          mInvalidatedRect.x - visibleRect.x,
>                          mInvalidatedRect.y - visibleRect.y,
>                          mInvalidatedRect.width,
>                          mInvalidatedRect.height,
>                          LOCAL_GL_RGBA,
>                          LOCAL_GL_UNSIGNED_BYTE,

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 452805 [details] [diff] [review]
CanvasLayerOGL use bind/release TexImage API


>+  nsRefPtr<GLContext> mCanvasGLSurface;

"mCanvasSurfaceAsGLContext" please -- it's long, but it's not confusing.  Rest of the patch looks ok, though it'll need updating based on other patches.

Comment 15

[Oleg Romashin (:romaxa)]

> 
> It occurs to me that this should probably be "CreateForNativePixmapSurface",
> because in general that's the only type of thing we'll be able to create an
> EGLImage from.  Calling it ASurface just implies some generic-ness that's not
> really true.

isn't it possible to create GL context for some image surface or windows/Mac OS X native surface?

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: CreateFromASurface Provide API. v2, mostly fixed vlad comments

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Fixed ThebesLayer use Bind/Release APi, v2, fixed vlad comments

Test with Native Surface and without native surface..

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: ImageLayerOGL use new API, v2 (fixed constant recreation and rebinding for native surface)

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: CanvasLayerOGL use bind/release TexImage API (to checking)

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #15)
> > 
> > It occurs to me that this should probably be "CreateForNativePixmapSurface",
> > because in general that's the only type of thing we'll be able to create an
> > EGLImage from.  Calling it ASurface just implies some generic-ness that's not
> > really true.
> 
> isn't it possible to create GL context for some image surface or windows/Mac OS
> X native surface?

Not really usefully; for a MacOS X target, "NativePixmap" would be correct still.

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 453049 [details] [diff] [review]
CreateFromASurface Provide API. v2, mostly fixed vlad comments


>+    /**
>+     * Try to create a GL context with surface for arbitrary gfxASurface
>+     * If surface not compatible this will return NULL
>+     *
>+     * @param aSurface surface to create a context for
>+     * @param aCreateContext if TRUE then it create FULL GL context
>+     *                       if FALSE then only bindable surface created
>+     * @return Context to use for this surface
>+     */
>+
>+    already_AddRefed<GLContext> CreateForASurface(gfxASurface *aSurface, PRBool aCreateContext = PR_FALSE);

Hmm, this API changed -- I really dislike the aCreateContext flag, given that you're on GLContextProvider.  Is there any reason that we don't have some other object that's a holder just for EGLImages?  Seems like you can put it on gfxXlibSurface directly even, and have it do the right magic to create on demand/destroy when the surface is destroyed.

This API is trying to do too much I think, leave the Create call for actually creating a GL context for a native pixmap, and obtaining an EGL image from a pixmap should be elsewhere.

>+    if (aSurface->GetType() != gfxASurface::SurfaceTypeXlib) {
>+        // Not implemented
>+        return nsnull;
>+    }
>+
>+#ifdef MOZ_X11
>+    gfxXlibSurface *xsurface = static_cast<gfxXlibSurface*>(aSurface);


Mote the surface type check into the #ifdef, because it doesn't make sense outside of it :-)

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 453051 [details] [diff] [review]
Fixed ThebesLayer use Bind/Release APi, v2, fixed vlad comments

This looks fine, though might need changes with a new API for EGLImages.  One comment below:

>+PRBool
>+ThebesLayerOGL::EnsureSurface()
> {
..
>+  mOffScreenSurface =
>+    gfxPlatform::GetPlatform()->
>+      CreateOffscreenSurface(gfxIntSize(mInvalidatedRect.width, mInvalidatedRect.height),
>+                             imageFormat);

We really want an Xlib surface here no X11, so maybe we should create one directly?  At some point I can see us switching to using image surfaces for example.  Though CreateOffscreen does some additional work... so maybe add an #ifdef MOZ_X11 section right after this with an NS_ASSERTION that the surface type is Xlib, so that we catch this in that case.

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: CreateFromASurface Provide API. v3, removed eglImage, renamed API

Comment 24

[Oleg Romashin (:romaxa)]

> ..
> >+  mOffScreenSurface =
> >+    gfxPlatform::GetPlatform()->
> >+      CreateOffscreenSurface(gfxIntSize(mInvalidatedRect.width, mInvalidatedRect.height),
> >+                             imageFormat);
> 
yep, that is correct for NON-QT/GTK code (win/mac) e.t.c...
We need to create Xlib surface here if it is X11, otherwise create imageSurface...

I was hoping that CreateOffscreenSurface will return some native GL compatible surface, otherwise image surface...

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 453076 [details] [diff] [review]
CreateFromASurface Provide API. v3, removed eglImage, renamed API

Looks good, thanks!  The EGLImage route is probably going to be a good bit faster for when we're just using Pixmaps as a source though, but this is probably ok for now.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Oh also -- add some documentation to BindTexImage/ReleaseTexImage in GLContext.h please

Comment 27

[Oleg Romashin (:romaxa)]

(In reply to comment #26)
> Oh also -- add some documentation to BindTexImage/ReleaseTexImage in
> GLContext.h please

Ok,

Btw, eglImage is 2x faster  comparing to normal surface

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nod; I definitely want the EGLImage path, but just not through this creating a non-GLContext GLContext :-)  The GL stuff all lives in thebes -- I think it's perfectly valid to have a GetEGLImage on gfxXlibSurface.

Comment 29

[Oleg Romashin (:romaxa)]

do you mean hack eglImage creation code directly into gfxXlibSurface class?

Comment 30

[Oleg Romashin (:romaxa)]

Broken build:
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1277286055.1277286324.4479.gz
Fixed in http://hg.mozilla.org/mozilla-central/rev/65bfcf60a068

Comment 31

[Oleg Romashin (:romaxa)]

Broken build:
http://tinderbox.mozilla.org/showlog.cgi?log=Mobile/1277286059.1277288322.15071.gz
Fixed in http://hg.mozilla.org/mozilla-central/rev/d5f400819ce6

Missing gfxASurface.h include

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 453076 [details] [diff] [review]
CreateFromASurface Provide API. v3, removed eglImage, renamed API

One more comment, didn't catch this first time around...  The logic in BindTexImage and ReleaseTexImage is unnecessarily complex (note the 4 different "return" and the many nested ifs):

>+    PRBool BindTexImage()
>+    {
>+        if (mBound)
>+            if (!ReleaseTexImage())
>+                return PR_FALSE;
>+
>+        if (mSurface) {
>+            EGLBoolean success;
>+            success = sEGLLibrary.fBindTexImage(mDisplay, (EGLSurface)mSurface,
>+                                                LOCAL_EGL_BACK_BUFFER);
>+            if (success == LOCAL_EGL_FALSE)
>+                return PR_FALSE;
>+        } else
>+            return PR_FALSE;
>+
>+        mBound = PR_TRUE;
>+        return PR_TRUE;
>+    }

Can be:

PRBool BindTexImage()
{
    if (!mSurface)
        return PR_FALSE;

    if (mBound && !ReleaseTexImage())
        return PR_FALSE;

    EGLBoolean success = sEGLLibrary.fBindTexImage(mDisplay, (EGLSurface)mSurface, LOCAL_EGL_BACK_BUFFER);
    if (success == LOCAL_EGL_FALSE)
        return PR_FALSE;

    mBound = PR_TRUE;
    return PR_TRUE;
}

And similarly for Release:

>+    PRBool ReleaseTexImage()
>+    {
>+        if (!mBound)
>+            return PR_TRUE;
>+
>+        if (!mDisplay)
>+            return PR_FALSE;

^ if (!mDisplay || !mSurface)

and then just:

>+            EGLBoolean success;
>+            success = sEGLLibrary.fReleaseTexImage(mDisplay, (EGLSurface)mSurface, LOCAL_EGL_BACK_BUFFER);
>+            if (success == LOCAL_EGL_FALSE)
>+                return PR_FALSE;
>+
>+        mBound = PR_FALSE;
>+        return PR_TRUE;
>+    }

Comment 33

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/9b0576208fa2
http://hg.mozilla.org/mozilla-central/rev/ac4fbacc524a
http://hg.mozilla.org/mozilla-central/rev/686a5a85d8c3
http://hg.mozilla.org/mozilla-central/rev/837221fdedbc
http://hg.mozilla.org/mozilla-central/rev/9a96aea54570