Due to some (presumed) weird refcount issues, eglDestroyImageKHR sometimes causes a crash in AndroidGraphicBuffer. It is commented out right now with the following annotation: /** * XXX: eglDestroyImageKHR crashes sometimes due to refcount badness (I think) * * If you look at egl.cpp (https://github.com/android/platform_frameworks_base/blob/master/opengl/libagl/egl.cpp#L2002) * you can see that eglCreateImageKHR just refs the native buffer, and eglDestroyImageKHR * just unrefs it. Somehow the ref count gets messed up and things are already destroyed * by the time eglDestroyImageKHR gets called. For now, at least, just not calling * eglDestroyImageKHR should be fine since we do free the GraphicBuffer below. */ We should get to the bottom of this if possible.

Comment on attachment 604427 [details] [diff] [review] patch Review of attachment 604427 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good to me. We might want to change AndroidDirectTexture/AndroidGraphicBuffer::Bind() to take the texture target as an argument intead of hardcoding it. That way someone could use it with GL_TEXTURE_2D still (like we do on m-c currently).

Comment on attachment 604427 [details] [diff] [review] patch Jeff, should I #ifdef ANDROID Copy2DExternalProgramType?

Comment on attachment 604427 [details] [diff] [review] patch Review of attachment 604427 [details] [diff] [review]: ----------------------------------------------------------------- Depending on the structure layout is pretty terrifying, but I don't have any objections about the actual implementation. ::: widget/android/AndroidGraphicBuffer.cpp @@ +438,5 @@ > return false; > > + EGLint eglImgAttrs[] = { LOCAL_EGL_IMAGE_PRESERVED_KHR, > + LOCAL_EGL_TRUE, LOCAL_EGL_NONE, LOCAL_EGL_NONE }; > + I would rather we use the system definitions here, but don't feel strongly.

Backed out because of the failing tests: https://hg.mozilla.org/projects/maple/rev/c405ba24c018

Dunno if it's useful or not (or for that matter whether it's true or not), but I missed noticing at first that both times, jsreftest-3 was failing by reporting OOM while reading the reftest manifest.

Comment on attachment 605200 [details] [diff] [review] patch We need to support optional shaders since the extension isn't available everywhere.

Comment on attachment 605200 [details] [diff] [review] patch I'd prefer an enum over a bool here. It will make reading SHADER_PROGRAM definitions more obvious

https://hg.mozilla.org/integration/mozilla-inbound/rev/4175fc853e16 https://hg.mozilla.org/integration/mozilla-inbound/rev/243cd4db5edd

Backed them both out (because I didn't know about either cause or separability) in https://hg.mozilla.org/integration/mozilla-inbound/rev/156a9cde0713 - OS X 10.6 and 10.7 were asserting in reftest, crashtest and jsreftest about ###!!! ASSERTION: not all programs were initialized!: 'programIndex == NumProgramTypes', file /builds/slave/m-in-osx64-dbg/build/gfx/layers/opengl/LayerManagerOGL.cpp, line 267 (though you have to ignore the way I starred that on several pushes as being something other than what it was).

I really don't think this patch is correct. As I explained over IRC the Qualcomm drivers explicitly say that this is the wrong thing to do. TEXTURE_2D should be used here.

Try run for e4d2cba31fe0 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=e4d2cba31fe0 Results (out of 81 total builds): success: 56 warnings: 20 failure: 5 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-e4d2cba31fe0

Comment on attachment 605889 [details] [diff] [review] Add conditional shaders and a Copy2DExternalProgramType for gralloc. My code works now with TEXTURE_2D. The bug was unrelated (missing glEnable on Mali). This patch is definitely wrong. Please don't land.

That's excellent news, it means we should be able to get this working on more phones!

snorp how much do we still care about this?

(In reply to Brad Lassey [:blassey] from comment #18) > snorp how much do we still care about this? We don't use this anywhere right now, so I guess we don't care at all.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #19) > (In reply to Brad Lassey [:blassey] from comment #18) > > snorp how much do we still care about this? > > We don't use this anywhere right now, so I guess we don't care at all. then wontfix. Perhaps we should remove this code and get it from hg log if we need it again.

(In reply to Benoit Girard (:BenWa) from comment #20) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #19) > > (In reply to Brad Lassey [:blassey] from comment #18) > > > snorp how much do we still care about this? > > > > We don't use this anywhere right now, so I guess we don't care at all. > > then wontfix. Perhaps we should remove this code and get it from hg log if > we need it again. I'm thinking about trying to revive it again, actually, if b2g gets a bit win with it when layer refactoring lands.