Two optimizations need to be enabled for Maemo6 EGL backend 1) UseBackingSurface = true 2) Fix EGLSurface creation and make LockSurface function working

Enable region clipping and transparent surface clearing.

Originally we had a bug about keeping EGL surface (drivers crash) But that was fixed... so we can create EGL surface once and destroy it in DTOR

Comment on attachment 497099 [details] [diff] [review] EGL optimize for Maemo6 > #ifdef MOZ_X11 >-static EGLConfig >-FindConfigForThebesXSurface(gfxASurface *aSurface, EGLSurface *aRetSurface) >+EGLSurface >+FindEGLSurfaceforXSurface(gfxASurface* aSurface, EGLConfig* aConfig) Is FindEGLSurfaceforXSurface really a good name for this? i.e. does it actually search for a EGL Surface or just create it? Also, "for" is not capitalized.

Direct rendering into locked texture with better clipping

Fix shader program type to RGB in case of backing surface available

KHR image detection is broken.... on maemo5 we have KHR image but don't have KHRimage from pixmap.

I have often error, caused by creating XPixmap without XSync and calling EGL functionality with non-existing pixmap.

Could you move all these reviews to jeff? He's doing most of the work on the mobile GL stuff lately..

Comment on attachment 498324 [details] [diff] [review] Probably obsolete, but nice to have this API working Need to recheck this patch, something wrong here

Comment on attachment 498327 [details] [diff] [review] UseBackingStore for Maemo6 + 16bpp format type Need recheck this patch once again... still have some problems with BltTexture on not-resized (created) texture.

Comment on attachment 498580 [details] [diff] [review] XSync before use EGL Please add a comment about why the XSync() is needed here.

> Please add a comment about why the XSync() is needed here. hmm, in order to avoid crash, or bad initialized EGL surface with this error message: https://bugzilla.mozilla.org/show_bug.cgi?id=593880#c0

I think just created X Surface not correctly registered and initialized for working with EGL... so XSync was the only solution to avoid GetGeometry: BadDrawable error message

I think it is better to get approval for this bug.. and get these patches integrated in order to make mozilla working on latest SGX drivers. with latest lock extensions et.c.

Comment on attachment 498988 [details] [diff] [review] BeginUpdate clip + transparency for backingSurface mode Can you give more detail on what this patch is for?

> BeginUpdate clip + transparency for backingSurface mode > Can you give more detail on what this patch is for? Actually this fix is similar to bug 621778, paint regions instead of region bounding rect. Another part is clearing surface before rendering transparent content... otherwise we will see overlapping Text thebesLayers in awesomebar and settings menu

dougt: why b-f was rejected here? we need this in order to get EGL working on recent SGX drivers... from another point of view it is almost NPODB...

2.0 is not blocking on maemo6. Please just request approval2.0 and you will get a stamp. also, yes, if this is all NPODB, no approval required.

Comment on attachment 498988 [details] [diff] [review] BeginUpdate clip + transparency for backingSurface mode > > //printf_stderr("BeginUpdate with updateRect [%d %d %d %d]\n", mUpdateRect.x, mUpdateRect.y, mUpdateRect.width, mUpdateRect.height); > > // We can only draw a rectangle, not subregions due to > // the way that our texture upload functions work. If > // needed, we /could/ do multiple texture uploads if we have > // non-overlapping rects, but that's a tradeoff. Update the comment. >- aRegion = nsIntRegion(mUpdateRect); >+ if (!mCreated) >+ aRegion = nsIntRegion(mUpdateRect); > It probably makes more sense to unify this with the code above. >- nsIntSize rgnSize = mUpdateRect.Size(); > if (!nsIntRect(nsIntPoint(0, 0), mSize).Contains(mUpdateRect)) { > NS_ERROR("update outside of image"); > return NULL; > mUpdateContext = new gfxContext(mBackingSurface); > } >+ >+ nsIntRegionRectIterator iter(aRegion); >+ const nsIntRect* rect; >+ while ((rect = iter.Next())) { >+ mUpdateContext->Rectangle(gfxRect(rect->x, rect->y, rect->width, rect->height), >+ PR_TRUE); >+ } >+ mUpdateContext->Clip(); gfxUtils::ClipToRegion()

Comment on attachment 499093 [details] [diff] [review] No clear required, Optimize DirectUpdate API to use lock extension >+ nsIntRegionRectIterator iter(aRegion); >+ const nsIntRect* rect; >+ while ((rect = iter.Next())) { >+ mUpdateContext->Rectangle(gfxRect(rect->x, rect->y, rect->width, rect->height), >+ PR_TRUE); >+ } >+ mUpdateContext->Clip(); ClipToRegion() >+ mUpdateContext->SetSource(aSurf); >+ mUpdateContext->SetOperator(gfxContext::OPERATOR_SOURCE); >+ mUpdateContext->Paint(); >+ mUpdateContext = nsnull; >+ UnlockSurface(); >+ mCreated = PR_TRUE; >+ return true; I don't really like unnecessary return's in the middle of functions.

Comment on attachment 502117 [details] [diff] [review] Use backing surface for maemo (EGL X) > mUpdateFormat = gfxASurface::FormatFromContent(GetContentType()); > >+ if (gUseBackingSurface) { >+ if (mUpdateFormat == gfxASurface::ImageFormatRGB24) { >+#ifdef MOZ_GFX_OPTIMIZE_MOBILE >+ mUpdateFormat = gfxASurface::ImageFormatRGB16_565; >+ mShaderType = RGBXLayerProgramType; >+#else >+ mUpdateFormat = gfxASurface::ImageFormatARGB32; >+ mShaderType = RGBALayerProgramType; >+#endif >+ } else { >+ mShaderType = RGBALayerProgramType; >+ } >+ CreateBackingSurface(gfxIntSize(aSize.width, aSize.height)); >+ return; >+ } I think this would be better without the return in the middle. > virtual ~TextureImageEGL() > { > GLContext *ctx = mGLContext; > if (ctx->IsDestroyed() || !NS_IsMainThread()) { >@@ -1246,32 +1261,32 @@ public: > > virtual void Resize(const nsIntSize& aSize) > { > NS_ASSERTION(!mUpdateContext, "Resize() while in update?"); > > if (mSize == aSize && mCreated) > return; > >+ mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > if (mBackingSurface) { > CreateBackingSurface(gfxIntSize(aSize.width, aSize.height)); > } else { >- mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > mGLContext->fTexImage2D(LOCAL_GL_TEXTURE_2D, > 0, > GLFormatForImage(mUpdateFormat), > aSize.width, > aSize.height, > 0, > GLFormatForImage(mUpdateFormat), > GLTypeForImage(mUpdateFormat), > NULL); >+ mCreated = PR_TRUE; > } I'm little worried about the different state (mCreated etc.) associated with these two styles of objects. Can you separate out the state associated with mBackingSurface contexts and those without?

>Can you separate out the state associated with mBackingSurface contexts and >those without? No sure that I fully understand what do you mean here... do you want if condition which does completely separate things for mBackingSurface and !mBackingSurface ?

Comment on attachment 502785 [details] [diff] [review] BeginUpdate clip + transparency for backingSurface mode. r2 >- // We can only draw a rectangle, not subregions due to >- // the way that our texture upload functions work. If >- // needed, we /could/ do multiple texture uploads if we have >- // non-overlapping rects, but that's a tradeoff. >- aRegion = nsIntRegion(mUpdateRect); >- >- nsIntSize rgnSize = mUpdateRect.Size(); Isn't this still true if !mBackingSurface?

oh, I though that 621778 covering this problem and already fixed...

Comment on attachment 503075 [details] [diff] [review] BeginUpdate clip + transparency for backingSurface mode. r3 > mUpdateRect = aRegion.GetBounds(); >+ if (!mBackingSurface) { >+ aRegion = nsIntRegion(mUpdateRect); >+ } It seems weird to update mUpdateRect based on aRegion.GetBounds() and then go back and update aRegion. Do we need both?

> It seems weird to update mUpdateRect based on aRegion.GetBounds() and then go > back and update aRegion. Do we need both? yes, mUpdateRect used in EndUpdate... aRegion used for understanding which area need to be repainted.

(In reply to comment #38) > yes, mUpdateRect used in EndUpdate... aRegion used for understanding which area > need to be repainted. Can we fix that?

> > need to be repainted. > Can we fix that? This is not bug, it is feature, because ThebesBuffer code does not track is texture fully painted or not, or is texture support region painting... it just suggesting Region in BeginUpdate and BeginUpdate can correct region according to texture state or region filling support.