Closed
Bug 592464
Opened 14 years ago
Closed 13 years ago
[OGL] fall back to image surfaces in EGL TextureImage code
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: vlad, Assigned: romaxa)
References
Details
Attachments
(2 files, 5 obsolete files)
48.90 KB,
patch
|
vlad
:
review-
|
Details | Diff | Splinter Review |
26.68 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #470936 -
Flags: review?(jones.chris.g)
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.
Attachment #470936 -
Flags: review?(jones.chris.g) → review-
Assignee | ||
Comment 2•14 years ago
|
||
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
Attachment #482017 -
Flags: review?(vladimir)
Assignee | ||
Comment 4•14 years ago
|
||
Btw, with this patch + pref("toolkit.browser.cachePixelY", 800); - we can get remote GL layers working on maemo...
Assignee | ||
Comment 5•14 years ago
|
||
Assignee: nobody → romaxa
Attachment #470936 -
Attachment is obsolete: true
Attachment #482017 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #486922 -
Flags: review?(jones.chris.g)
Attachment #482017 -
Flags: review?(vladimir)
Assignee | ||
Comment 6•14 years ago
|
||
Comment on attachment 486922 [details] [diff] [review]
EGL backend rework, no backing surface
Bad version
Attachment #486922 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 7•14 years ago
|
||
This should be more easy for review
Attachment #486922 -
Attachment is obsolete: true
Attachment #487130 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #487130 -
Attachment is obsolete: true
Attachment #487130 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #487143 -
Attachment is obsolete: true
Attachment #487573 -
Flags: review?(vladimir)
Assignee | ||
Updated•14 years ago
|
Attachment #487573 -
Flags: review?(jones.chris.g)
Reporter | ||
Comment 10•14 years ago
|
||
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).
Attachment #487573 -
Flags: review?(vladimir) → review-
Assignee | ||
Comment 11•14 years ago
|
||
> >+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
Reporter | ||
Comment 12•14 years ago
|
||
(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?
Assignee | ||
Comment 13•14 years ago
|
||
not sure, but on maemo5 it says that KHR things are available, but it seems does not work... not sure... will double check it
Reporter | ||
Comment 14•14 years ago
|
||
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.
Attachment #491372 -
Flags: review?(jmuizelaar)
Comment 15•14 years ago
|
||
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.
Attachment #491372 -
Flags: review?(jmuizelaar) → review+
Updated•14 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Comment 16•14 years ago
|
||
can we get an approval for b-f and get it in?
Updated•14 years ago
|
tracking-fennec: ? → 2.0b4+
Comment 17•14 years ago
|
||
Assignee | ||
Comment 18•14 years ago
|
||
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
Attachment #487573 -
Flags: review?(jones.chris.g)
Updated•14 years ago
|
Summary: fall back to image surfaces in EGL TextureImage code → [OGL] fall back to image surfaces in EGL TextureImage code
Is there anything left to do here? GL certainly works on android, more or less.
Updated•14 years ago
|
tracking-fennec: 2.0b4+ → 2.0next+
Comment 20•14 years ago
|
||
(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.
Updated•14 years ago
|
tracking-fennec: 2.0next+ → 7+
Updated•13 years ago
|
tracking-fennec: 7+ → 8+
Updated•13 years ago
|
tracking-fennec: 8+ → +
Comment 21•13 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•