Closed Bug 571832 Opened 10 years ago Closed 9 years ago

GL ThebesLayer rendering layout on X-System terribly slow.

Categories

(Core :: Graphics, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(5 files, 8 obsolete files)

3.35 KB, patch
vlad
: review+
Details | Diff | Splinter Review
8.14 KB, patch
vlad
: review+
Details | Diff | Splinter Review
4.17 KB, patch
vlad
: review+
Details | Diff | Splinter Review
3.84 KB, patch
Details | Diff | Splinter Review
18.24 KB, patch
vlad
: review+
Details | Diff | Splinter Review
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.
Summary: GL ThebesLayer rendering on X-System terribly slow. → GL ThebesLayer rendering layout on X-System terribly slow.
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
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?
(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.
Attached patch WIP2, Seems to work (obsolete) — Splinter Review
Added GLSurface class (Bind/Release, init/destroy)
Added KHR extension handling,
Fixed layer rendering
Some cleanup.
Attachment #451116 - Attachment is obsolete: true
Attachment #451295 - Flags: feedback?(bas.schouten)
Minor issue with glEGLImageTargetTexture2DOES binding.. but feedback about structure and API is needed
Attachment #451295 - Attachment is obsolete: true
Attachment #451603 - Flags: feedback?
Attachment #451295 - Flags: feedback?(bas.schouten)
Attachment #451603 - Flags: feedback? → feedback?(bas.schouten)
Attachment #451603 - Attachment is obsolete: true
Attachment #451700 - Flags: review?(bas.schouten)
Attachment #451603 - Flags: feedback?(bas.schouten)
Attached patch Switch ThebesLayer to new API (obsolete) — Splinter Review
Attachment #451701 - Flags: review?(bas.schouten)
I will try to have this reviewed tomorrow (20100617).
Attachment #451701 - Attachment is obsolete: true
Attachment #452804 - Flags: review?(bas.schouten)
Attachment #451701 - Flags: review?(bas.schouten)
Attachment #452805 - Flags: review?(bas.schouten)
Attached patch ImageLayerOGL use new API (obsolete) — Splinter Review
Attachment #452807 - Flags: review?(bas.schouten)
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 */
Attachment #451700 - Flags: review-
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 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.
Attachment #452805 - Flags: review?(bas.schouten) → review+
> 
> 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?
Attachment #451700 - Attachment is obsolete: true
Attachment #453049 - Flags: review?(bas.schouten)
Attachment #451700 - Flags: review?(bas.schouten)
Test with Native Surface and without native surface..
Attachment #452804 - Attachment is obsolete: true
Attachment #453051 - Flags: review?(vladimir)
Attachment #452804 - Flags: review?(bas.schouten)
Attachment #452807 - Attachment is obsolete: true
Attachment #453052 - Flags: review?(bas.schouten)
Attachment #452807 - Flags: review?(bas.schouten)
(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 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 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.
Attachment #453051 - Flags: review?(vladimir) → review+
Attachment #453052 - Flags: review?(bas.schouten) → review+
Attachment #453049 - Attachment is obsolete: true
Attachment #453076 - Flags: review?(vladimir)
Attachment #453049 - Flags: review?(bas.schouten)
> ..
> >+  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 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.
Attachment #453076 - Flags: review?(vladimir) → review+
Oh also -- add some documentation to BindTexImage/ReleaseTexImage in GLContext.h please
(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
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.
do you mean hack eglImage creation code directly into gfxXlibSurface class?
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;
>+    }
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.