Comment on attachment 606958 [details] [diff] [review] patch Patch doesn't work yet. Nothing gets blitted. Screen is completely blank. Looking for hints why not.

GL provider paths which require binding some non-FBO back-buffer are currently somewhat in disrepair, so it may just be that. The patch file doesn't the function names for the hunks (everything says 'public:'), so it's hard to tell what's going where without applying it. I'll take a closer look tomorrow, and see if anything jumps out at me though. I/we're looking at refactoring the providers presently, since a number of the paths have collected significant dust.

Comment on attachment 606958 [details] [diff] [review] patch Review of attachment 606958 [details] [diff] [review]: ----------------------------------------------------------------- Without debugging it I don't know off the top of my head exactly why it isn't working, but I noticed some stuff looking over the patch. ::: gfx/gl/GLContextProviderEGL.cpp @@ +1971,5 @@ > > +#ifdef MOZ_WIDGET_GONK > + if (mGraphicBuffer != nsnull) { > + mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, nsnull); IIRC, you want to do create the EGLImageKHR and call glEGLImageTargetTexture2DOES() after the unlock-and-post, not before the lock. @@ +2041,5 @@ > mIsLocked = false; > + > +#ifdef MOZ_WIDGET_GONK > + if (mGraphicBuffer != nsnull) { > + mGraphicBuffer->unlock(); Does this function post the buffer back to the surface? I haven't used the GraphicBuffer functions much (usually I used the lower-level stuff), but generally on Android you have to unlock the buffer and then post it back to the surface (to make double buffering work). @@ +2172,5 @@ > + > + const int eglImageAttributes[] = { EGL_IMAGE_PRESERVED_KHR, LOCAL_EGL_TRUE, > + LOCAL_EGL_NONE, LOCAL_EGL_NONE }; > + mImageKHR = sEGLLibrary.fCreateImageKHR(EGL_DISPLAY(), > + EGL_NO_CONTEXT, Why not sEGLLibrary.fGetCurrentContext() instead of EGL_NO_CONTEXT? @@ +2181,5 @@ > + printf_stderr("couldn't create EGL image: ERROR (0x%04x)\n", sEGLLibrary.fGetError()); > + return false; > + } > + mGLContext->fBindTexture(LOCAL_GL_TEXTURE_2D, mTexture); > + sEGLLibrary.fImageTargetTexture2DOES(LOCAL_GL_TEXTURE_2D, mImageKHR); Again, I think you need to do this after the unlock-and-post happens.

Oh, and don't forget the standard stuff: checking logcat to make sure there weren't any permission errors on the relevant /dev devices, for example. (I don't know whether Gecko is running is root on gonk...)

> IIRC, you want to do create the EGLImageKHR and call > glEGLImageTargetTexture2DOES() after the unlock-and-post, not before the > lock. surfaceflinger seems to do it like this as well and I found some documentation that CreateImageKHR is allowed to be pretty slow. If I understand correctly what happens inside the driver this basically binds the buffer to the texture (it doesn't copy, it binds), so it should survive unlock-and-post repeatedly. I _think_. > Does this function post the buffer back to the surface? I haven't used the > GraphicBuffer functions much (usually I used the lower-level stuff), but > generally on Android you have to unlock the buffer and then post it back to > the surface (to make double buffering work). GraphicBuffer is basically just a shim over gralloc. It gets the buffer and then maps it into virtual memory and flushes it back to the gpu. It doesn't do any double buffering. We don't need to. We are modifying a texture and then flushing it (post) and then using it. Again, I _think_ this matches what android does. > Why not sEGLLibrary.fGetCurrentContext() instead of EGL_NO_CONTEXT? Documentation says to use NO_CONTEXT and surfaceflinger does too. > > Again, I think you need to do this after the unlock-and-post happens.

(In reply to Andreas Gal :gal from comment #7) > GraphicBuffer is basically just a shim over gralloc. It gets the buffer and > then maps it into virtual memory and flushes it back to the gpu. It doesn't > do any double buffering. We don't need to. We are modifying a texture and > then flushing it (post) and then using it. Again, I _think_ this matches > what android does. Ah yes, I forgot: GraphicBuffer is a gralloc interface, not an android Surface interface. Probably jrmuizel is right then: it's a problem somewhere in the Gecko code path leading up to these calls.

(In reply to Patrick Walton (:pcwalton) from comment #8) > Probably jrmuizel is right then: it's a problem somewhere in the Gecko code > path leading up to these calls. Err, jgilbert.

I think I lied about when surface flinger makes and tears down the ImageKHR stuff. Trying that now.

And thanks Jeff. I could definitely use help with that. I am definitely a little lost in the provider code. Its ... hairy.

I updated the patch. It still doesn't display anything, and gecko is leaking memory like mad if I interact with the screen (pan the lockscreen around blindly).

This is superseded by all the other gralloc bugs.

Can we just close this, then?

Yes. RESOLVED -> rand(6)