592464 - [OGL] fall back to image surfaces in EGL TextureImage code

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: basic texture image fallback

There's no fallback here, leading to no drawing if a platform doesn't implement the GLContext surface stuff.  This just adds basic gfxImageSurface context.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 470936 [details] [diff] [review]
basic texture image fallback

This would be a lot simpler and cleaner if you created BasicTextureImageEGL (or whatever class name) as a subclass of BasicTextureImage (see GLContextProviderGLX.cpp, e.g.), overrode GLContext::CreateBasicTextureImage, then fell back on GLContext::CreateTextureImage if we can't use texture-from-pixmap.

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Fallback to image surface TextureImage code in EGL backend

This is what I have now... it seems to work for BasicBufferOGL code.
export USE_BACKSURF=1 will enable attempt to bind texture with X surface, but that does not work very well, because ThebesBufferLayer code destroying and re-creating TextureImage too often, and that cause screen blinking...

With current patch we only have problem with bug 602523

Comment 4

[Oleg Romashin (:romaxa)]

Btw, with this patch + pref("toolkit.browser.cachePixelY", 800); - we can get remote GL layers working on maemo...

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: EGL backend rework, no backing surface

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 486922 [details] [diff] [review]
EGL backend rework, no backing surface

Bad version

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: More simple version, only image surface related changes

This should be more easy for review

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Fixed bug 608497 problem

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Works for Maemo5 Qt/Gtk webgl works too, no optimizations enabled, glTexImage2d upload by default

Comment 10

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

Comment on attachment 487573 [details] [diff] [review]
Works for Maemo5 Qt/Gtk webgl works too, no optimizations enabled, glTexImage2d upload by default

Note that the GLX context provider isn't the best one to model code after -- it's different than all the others (including EGL), and I really dislike its giant static Create function that does magic depending on arguments.  In this case though, the work seems common enough that it's probably ok.

>diff --git a/gfx/thebes/GLContextProviderEGL.cpp b/gfx/thebes/GLContextProviderEGL.cpp
>--- a/gfx/thebes/GLContextProviderEGL.cpp
>+++ b/gfx/thebes/GLContextProviderEGL.cpp
>@@ -138,18 +138,22 @@ public:
> 
> #endif
> 
> #include "gfxASurface.h"
> #include "gfxImageSurface.h"
> #include "gfxPlatform.h"
> #include "GLContextProvider.h"
> #include "nsDebug.h"
>+#include "nsThreadUtils.h"
> 
> #include "nsIWidget.h"
>+#include <sys/time.h>

What's this for?

>+static bool useBackingSurface = getenv("USE_BACKSURFACE") != 0;

If this is still needed, PR_GetEnv and MOZ_GL_USE_BACKING_SURFACE or something, but can it just go away?


>+#if (MOZ_PLATFORM_MAEMO == 6)
>+#define HAS_TEXTURE_SIZE_LIMIT
>+#define MAX_TEXTURE_WIDTH 2000
>+#define MAX_TEXTURE_HEIGHT 2000
>+#endif

Why is this just on maemo 6?  Should query the value GL_MAX_TEXTURE_SIZE and use that instead.

>+#if (MOZ_PLATFORM_MAEMO != 5)
>         // note the extra space -- this ugliness tries to match
>         // EGL_KHR_image in the middle of the string, or right at the
>         // end.  It's a prefix for other extensions, so we have to do
>         // this...
>         PRBool hasKHRImage;
>         if (strstr(extensions, "EGL_KHR_image ") ||

...

Why is this not done on Maemo5?  Do we not want to use some of these extensions there?  We should probably just avoid trying to use GL at all on Maemo 5, I don't really see any reason to support it there.

>+    static already_AddRefed<GLContextEGL>
>+    CreateGLContext(const ContextFormat& format,
>+                    EGLSurface surface,
>+                    EGLConfig config,
>+                    GLContextEGL *shareContext,
>+                    PRBool aIsOffscreen = PR_FALSE,
>+                    gfxASurface *pixmap = nsnull)

Get rid of the 'pixmap' parameter -- callers that need to call HoldSurface should do so directly, instead of passing it to this function to do it for them.

>@@ -623,16 +718,19 @@ public:
> #else
>                 succeeded = PR_FALSE;
> #endif
>             } else {
>                 succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
>                                                      mSurface, mSurface,
>                                                      mContext);
>             }
>+            succeeded = sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
>+                                                 mSurface, mSurface,
>+                                                 mContext);

Isn't this the same call as in the else block above?  This doesn't really make sense to me, either that else block should just be removed, but what happens if the previous bit fails and sets succeeded to false?

>     virtual PRBool TextureImageSupportsGetBackingSurface()
>     {
>-#ifdef MOZ_WIDGET_QT
>-        return (gfxASurface::SurfaceTypeXlib ==
>-            gfxPlatform::GetPlatform()->ScreenReferenceSurface()->GetType());
>-#else
>-        return PR_TRUE;
>-#endif
>+        return PR_FALSE;
>     }

Pretty large change -- can you explain this?

>+class TextureImageEGL
>+    : public TextureImage
> {
> public:
>-    TextureImageEGL(GLuint aTexture,
>-                    const nsIntSize& aSize,
>-                    ContentType aContentType,
>-                    GLContext* aContext,
>-                    GLContextEGL* aImpl,
>-                    PRBool aIsRGB)
>-        : TextureImage(aTexture, aSize, aContentType, aIsRGB)
>-        , mGLContext(aContext)
>-        , mImpl(aImpl)
>-    { }

Where'd the constructor go?  Seems like it's buried way down below, bring it back up to the top...

>     virtual gfxContext* BeginUpdate(nsIntRegion& aRegion)
>     {
>         NS_ASSERTION(!mUpdateContext, "BeginUpdate() without EndUpdate()?");
>-        if (mImpl) {
>-            mUpdateContext = new gfxContext(mImpl->mThebesSurface);
>-            // TextureImageEGL can handle updates to disparate regions
>-            // aRegion = aRegion;
>-        } else {
>+
>+        // determine the region the client will need to repaint
>+        if (!mTextureInited) {
>+            // if the texture hasn't been initialized yet, force the
>+            // client to paint everything
>+            mUpdateRect = nsIntRect(nsIntPoint(0, 0), mSize);
>+        }
>+        else {

No hanging 'else' please -- on the same line as the }

>             mUpdateRect = aRegion.GetBounds();
>-            if (!mUpdateSurface) {
>-                NS_ASSERTION(mUpdateRect.x == 0 && mUpdateRect.y == 0,
>-                             "Initial update has to be full surface!");
>-                mUpdateSurface = new gfxImageSurface(gfxIntSize(mUpdateRect.width, mUpdateRect.height),
>-                                                     mContentType == gfxASurface::CONTENT_COLOR
>-                                                     ? gfxASurface::ImageFormatRGB24
>-                                                     : gfxASurface::ImageFormatARGB32);
>+        }
>+        // the basic impl can't upload updates to disparate regions,
>+        // only rects
>+        aRegion = nsIntRegion(mUpdateRect);

So we are universally removing the ability to draw to region-shaped areas?  This seems like a net loss, if we're using an Xlib pixmap bound with texture-from-pixmap, isn't it?

>+        if (mBackingSurface) {
>+            if (sEGLLibrary.HasKHRImageLock()) {
>+                nsRefPtr<gfxASurface> surface = GetLockSurface();
>+                mUpdateContext = new gfxContext(surface);
>+                mUpdateContext->Rectangle(gfxRect(mUpdateRect.x, mUpdateRect.y, mUpdateRect.width, mUpdateRect.height));
>+                mUpdateContext->Clip();

Hm, not sure why we need to collapse the region to a rectangle -- if we have the full surface locked, we should be able to update any parts of it.  You also shouldn't need to set the clip here, I believe that there's a guarantee that the caller won't draw outside the region.

>+    virtual already_AddRefed<gfxImageSurface> GetLockSurface()
>+    {
>+        if (mIsLocked) {
>+            printf("Cannot lock surface twice\n");

No printfs, use NS_WARNING or similar here.

>+            return nsnull;
>+        }
>+        if (!sEGLLibrary.HasKHRImageLock()) {
>+            printf("Don't have lock extension\n");

This is going to be triggered a lot, get rid of the printf/warning/etc.  Seems like this is a normal path if callers aresupposed to call GetLockSurface unconditionally.

> already_AddRefed<GLContext>
> GLContextProviderEGL::CreateForWindow(nsIWidget *aWidget)
> {
>     if (!sEGLLibrary.EnsureInitialized()) {
>         return nsnull;
>     }
> 
>+    int depth = 24;

This isn't a valid initial depth; it'll really only be correct on win32 with ANGLE or native EGL.  Then again, maybe it doesn't matter, since currently we always pass BasicRGB24.  The actual format should be updated correctly, don't think this creation format is used anywhere though we should fix it at some point.
>+#if defined(XP_WIN) || defined(MOZ_PLATFORM_MAEMO)
>     glContext->SetIsDoubleBuffered(PR_TRUE);
> #endif

Should do this unconditionally for window surfaces.  It's always going to be true for EGL.


> #ifdef XP_WIN
> already_AddRefed<GLContextEGL>
> GLContextEGL::CreateEGLWin32OffscreenContext(const gfxIntSize& aSize,
>                                              const ContextFormat& aFormat)

This didn't need any changes?

Looks mostly fine, just needs some fixes and testing on various other platforms (specifically on Android and EGL on Win32).

Comment 11

[Oleg Romashin (:romaxa)]

> >+static bool useBackingSurface = getenv("USE_BACKSURFACE") != 0;
> 
> If this is still needed, PR_GetEnv and MOZ_GL_USE_BACKING_SURFACE or something,

yes, I want to keep possibility to test SLOW (TexImage2D upload) and will do option to disable backing surface


> Why is this not done on Maemo5?  Do we not want to use some of these extensions
> there?  We should probably just avoid trying to use GL at all on Maemo 5, I
> don't really see any reason to support it there.

Maemo5 is the only mobile public platform from us, and fixing GL problems on this platform helps a lot with solving same problems on next platform (meego)
Also it would be nice to have texture scrolling and video rendering (yuv + composite) in GPU


>       gfxPlatform::GetPlatform()->ScreenReferenceSurface()->GetType());
> >-#else
> >-        return PR_TRUE;
> >-#endif
> >+        return PR_FALSE;
> >     }
> 
> Pretty large change -- can you explain this?

Backing surface enabling SurfaceBufferOGL, which is very broken and don't know who is testing that... I'll double check that patch but IIUC nobody playing with that creature.




I'll try fix the rest of comments and attach new patch

Comment 12

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

(In reply to comment #11)
> > Why is this not done on Maemo5?  Do we not want to use some of these extensions
> > there?  We should probably just avoid trying to use GL at all on Maemo 5, I
> > don't really see any reason to support it there.
> 
> Maemo5 is the only mobile public platform from us, and fixing GL problems on
> this platform helps a lot with solving same problems on next platform (meego)
> Also it would be nice to have texture scrolling and video rendering (yuv +
> composite) in GPU

Ok, that makes sense -- doesn't answer the first part of the question though, why disable all the extension checks on Maemo5?

Comment 13

[Oleg Romashin (:romaxa)]

not sure, but on maemo5 it says that KHR things are available, but it seems does not work... not sure... will double check it

Comment 14

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

Attachment: partial update

here's a part of the original patch; largely everything in TextureImage, and none of the context creation bits.  Also most of the pieces actually using EGL_lock_surface are #ifdef'd out or removed, because I can't test with those.

This seems to fix a number of issues, but introduces some corruption that I can't figure out.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 491372 [details] [diff] [review]
partial update


>+        // Note that if we start using RGB565 here, we'll need to
>+        // watch for a) setting mIsRGBFormat to TRUE; and b) getting
>+        // the stride right (default GL stride is always 4-byte aligned,
>+        // cairo I believe will pack the 2 bytes together).

cairo's byte strides are also always multiples of 4. i.e. rows will never break the 4-byte word boundary.

I haven't really looked over this in too much detail, but it seems like a better place to start from than the existing code. So I'm happy to get it in sooner rather than later.

Comment 16

[Oleg Romashin (:romaxa)]

can we get an approval for b-f and get it in?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/40b37c64a240

Comment 18

[Oleg Romashin (:romaxa)]

Comment on attachment 487573 [details] [diff] [review]
Works for Maemo5 Qt/Gtk webgl works too, no optimizations enabled, glTexImage2d upload by default

cleanup request queue... this was landed partially... need new patch for fix bits on maemo5/6

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Is there anything left to do here?  GL certainly works on android, more or less.

Comment 20

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

(In reply to comment #19)
> Is there anything left to do here?  GL certainly works on android, more or
> less.

Um, it doesn't work on Android here (I'm getting bug 606794), and actually the whole code in CreateBackingSurface in this patch is #ifdef MOZ_X11, and I'm trying to figure now how to make it work on Android i.e. remove the X11 dependency. Until then, all the fixes made in this area do not make a difference on Android.

Comment 21

[Benoit Girard (:BenWa)]

Patch landed and is on the tree. If we need additional work to be done for Android we should file a follow up as we see the need but I don't believe that this is currently the case.