819609 - Be defensive about videos we send to the HW decoder (video app crashes with low pmem (dequeueBuffer() needs proper error handling))

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Inder]

STR:
1. Put few high resolution videos in /sdcard/Movies
2. Start video app
Video app crashes.
For a more consistent way to reproduce the issue: Reduce pmem size to 2MB or so and video app crashes with lower resolution(HVGA) videos as well.

Diagnosis:
Debugged it to find the problem in GrallocBufferActor::Create() doesn't do any error handling. This function gets eventually called from dequeueBuffer() in GonkNativeWindow.cpp.
When pmem is low then GraphicBuffer() fails and initCheck() returns an error but doesn't look like GrallocBufferActor::Create() is doing anything to propagate the error to dequeueBuffer().
See: https://github.com/mozilla/mozilla-central/blob/beta/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#L169

As a result there is a bad dereference somewhere and dequeueBuffer() never returns. There are couple of other places in that file where we are creating GraphicBuffer but not doing any error handling.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh, vomit.  The core code *is* actually propagating the errors correctly, although it's fairly confusing (my fault for that).

The problem is that the error-handling code in GonkNativeWindow::dequeueBuffer() is plain wrong.  We would crash any time allocation actually failed.  That code gave me a strong sense of deja vu, and the reason is that I already fixed that bug!  Just the patch wasn't uplifted correctly.  So let me fix that wagon and we can retest here.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f325888eb10

I'm confident enough that that's the fix to close this.  Please reopen if this doesn't do the trick.

Comment 3

[Diego Wilson [:diego]]

Good catch Inder! Does this solve the problem in bug 804802 ?

Comment 4

[Inder]

Chris,
   I am still seeing the issue with this patch. The problem is it never returns back from the call to AllocSurfaceDescriptorGralloc() in dequeueBuffer(). The error check you added is after this function call which is never reached.

Comment 5

[Doug Turner (:dougt)]

over to cjones.  please unassign if you are not able to investigate (or reassign as appropriate).

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Don't try to send too-large videos to hardware video decoders

This sure doesn't feel like the right way to solve this bug but we have a priority request to stop the badness.  Followup work is already filed.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: build.prop hackery

Just demonstrates the build properties that will need to be updated to enable the new code here.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It appears that the fallback google SW decoder is next to useless in practice.  This patch might end up causing videos that previously barely used to play to hit the SW decoder and not play.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://github.com/mozilla-b2g/android-device-otoro/commit/2825a9772f16800eba1c1901fa71a74ba464e9ce
https://github.com/mozilla-b2g/android-device-unagi/commit/eae5c49bc5df4ba54bba0675c5e3836a15995381
https://hg.mozilla.org/integration/mozilla-inbound/rev/bda0d77acf0c
https://hg.mozilla.org/releases/mozilla-aurora/rev/c042ea08a2b4
https://hg.mozilla.org/releases/mozilla-beta/rev/3fcae8234ba9

Comment 10

[Michael Vines [:m1] [:evilmachines]]

Sadness.  There surely is another way for I believe there is good in this world.  

Do we shutdown the camera on home key these days?  If not then it's still consuming pmem and I bet we still can hit the bug.

Inder, can you show us the light?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Michael Vines [:m1] from comment #10)
> Do we shutdown the camera on home key these days?  If not then it's still
> consuming pmem and I bet we still can hit the bug.
> 

We do.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Although even if we didn't, it would likely be easy to construct videos that passed this dumb check but still exhausted pmem.

I agree that there must be another way!

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bda0d77acf0c