1032059 - Crash when playing video on Samsung Galaxy S II due to problem in libstagefright

Status: RESOLVED WORKSFORME

(Core :: Audio/Video: Playback, defect)

Description

[David Cook [:dncook]]

Attachment: Workaround for ColorConverter, version 1

This is a follow-up from from 845729. The cause for the crash is an assertion in ColorConverter::convert() from certain builds of libstagefright.so. ColorConverter::isValid() will return true when the source format is QOMX_COLOR_FormatYUV420PackedSemiPlanar64x32Tile2m8ka, (0x7fa30c03) so the code from bug 803394 chooses the hardware codec using that format. When ColorConverter::convert() is called with the same format, it raises an asssertion.

There are four obvious workarounds I can think of.
1. Replace the color conversion routines with new ones, either written from scratch or from ffmpeg or VLC.
2. Skip ColorConverter on problem devices, and either fall back to libI420colorconvert.so or choose a different codec.
3. Blocklist all hardware stagefright codecs on problem devices
4. Change the priority of color conversion routines, check libI420colorconvert before ColorConverter. It looks like libI420colorconvert.so can handle color format 0x7fa30c03, which is good.

I have attached a patch that combines workarounds 2 and 4. Does this sound like a good approach? I did my testing on the T-Mobile US version of the Samsung Galaxy S II, (SGH-T989) running Android 4.0.4. The model strings in the blocklist are from crash reports matching the signatures on bug 845729. The models are variations on the Samsung Galaxy S II, Samsung Galaxy Note, Samsung Galaxy Tab. (and Samsung Galaxy S Blaze, Samsung Exhilirate)

Comment 1

[cajbir (:cajbir)]

Comment on attachment 8447830 [details] [diff] [review]
Workaround for ColorConverter, version 1

I think this is more Edwin's domain than mine.

Comment 2

[David Cook [:dncook]]

Review ping?

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8447830 [details] [diff] [review]
Workaround for ColorConverter, version 1

Review of attachment 8447830 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the wait. We had some major-ish colour conversion changes in the pipeline so I was putting this off, but that has stalled for now. LGTM.

Comment 4

[Carsten Book [:Tomcat]]

Hi, could you provide a try link for this patch thanks!

Comment 5

[David Cook [:dncook]]

I don't think I have access to the tryserver

Comment 6

[Carsten Book [:Tomcat]]

(In reply to dncook from comment #5)
> I don't think I have access to the tryserver

edwin could you help here ?

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://tbpl.mozilla.org/?tree=Try&rev=c61ecda594d6

Comment 8

[cajbir (:cajbir)]

What's the status of this? Was the patch suppose to land?

Comment 9

[David Cook [:dncook]]

I'm not sure where this sits, but if needed, I could refresh the patch to deal with bitrot and test it again. What are the next steps?

Comment 10

[Chris Peterson [:cpeterson]]

Edwin, can dncook's Samsung fix land now? Or do you want to finish the other color conversion changes first?

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Yes! We should land this if it's still an issue.

Comment 12

[David Cook [:dncook]]

I recently upgraded my phone from ICS to JB, which fixed the underlying issue, but just before that, the latest Firefox was still triggering the same crash.

Comment 13

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Upgrade to JB fixes the issue.