Closed Bug 712716 Opened 8 years ago Closed 7 years ago

AndroidGraphicBuffer doesn't call eglDestroyImageKHR

Categories

(Firefox for Android :: General, defect, P3)

ARM
Android
defect

Tracking

()

RESOLVED WONTFIX
Firefox 14
Tracking Status
fennec - ---

People

(Reporter: snorp, Unassigned)

References

Details

(Whiteboard: maple)

Attachments

(2 files, 2 obsolete files)

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.
Priority: -- → P3
tracking-fennec: --- → 11+
Assignee: snorp → bgirard
Whiteboard: maple
Attached patch patch (obsolete) — Splinter Review
Attached patch patchSplinter Review
Attachment #604423 - Attachment is obsolete: true
Attachment #604427 - Flags: review?(snorp)
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).
Attachment #604427 - Flags: feedback+
Attachment #604427 - Flags: review?(snorp) → review+
Comment on attachment 604427 [details] [diff] [review]
patch

Jeff, should I #ifdef ANDROID Copy2DExternalProgramType?
Attachment #604427 - Flags: review?(jmuizelaar)
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.
Attachment #604427 - Flags: review?(jmuizelaar) → review+
Backed out because of the failing tests: https://hg.mozilla.org/projects/maple/rev/c405ba24c018
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #605200 - Flags: review?(jmuizelaar)
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
Attachment #605200 - Flags: review?(jmuizelaar) → review-
Attachment #605889 - Flags: review?(jmuizelaar)
Attachment #605200 - Attachment is obsolete: true
Blocks: 729538
Attachment #605889 - Flags: review?(jmuizelaar) → review+
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).
Blocks: 736801
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.
Attachment #605889 - Flags: review-
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?
tracking-fennec: 11+ → ?
(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.
tracking-fennec: ? → -
(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.
Assignee: bgirard → nobody
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
(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.
You need to log in before you can comment on or make changes to this bug.