784329 - Stagefright: Galaxy Nexus hardware decoder video is blank. Need OMX_TI_COLOR_FormatYUV420PackedSemiPlanar color conversion.

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

adb shell am start -a android.intent.aciton.VIEW -n org.mozilla.fennec/.App -d http://people.mozilla.com/~atrain/mobile/tests/test.mp4

I/OMXClient( 2574): Using client-side OMX mux.
I/OMXCodec( 2574): [OMX.TI.DUCATI1.VIDEO.DECODER] AVC profile = 66 (Baseline), level = 30
I/OMXCodec( 2574): [OMX.TI.DUCATI1.VIDEO.DECODER] video dimensions are 320 x 178
I/OMXCodec( 2574): [OMX.TI.DUCATI1.VIDEO.DECODER] Crop rect is 320 x 178 @ (0, 0)

This worked on mozilla-inbound yesterday (08/21), this is a new regression.

--
Samsung Galaxy Nexus (Android 4.1.1)
20120821054201
http://hg.mozilla.org/integration/mozilla-inbound/rev/eb4392d9bc48

Comment 1

[Chris Peterson [:cpeterson]]

This bug is likely fallout from bug 782508.

Comment 2

[Chris Peterson [:cpeterson]]

This bug is definitely fallout from bug 782508. If I force Stagefright to use the software decoder, then the video plays. If I force Stagefright to use the hardware decoder, then video fails to play.

Curiously, the hardware and software decoders disagree on the video's dimensions. The hardware decoder also logs errors about "Unknown video color format: 7f000100". (I am testing a Galaxy Nexus running Jelly Bean.)


* With kHardwareCodecsOnly:

I/OmxPlugin(30096): XXX CreateDecoder: aMimeChars="video/mp4"
I/OmxPlugin(30096): Init: MediaExtractor found videoTrackIndex=0, videoMime="video/avc"
I/OmxPlugin(30096): Init: MediaExtractor found audioTrackIndex=1, audioMime="audio/mp4a-latm"
I/OmxPlugin(30096): Init: video duration=185819152 us
I/OmxPlugin(30096): Init: audio duration=185829297 us
I/OmxPlugin(30096): stride not available, assuming width
I/OmxPlugin(30096): slice height not available, assuming height
I/OmxPlugin(30096): rotation not available, assuming 0
I/OmxPlugin(30096): width: 320 height: 178 component: OMX.TI.DUCATI1.VIDEO.DECODER format: 2130706688 stride: 320 sliceHeight: 178 rotation: 0
I/OmxPlugin(30096): channelCount: 2 sampleRate: 44100
I/OmxPlugin(30096): stride not available, assuming width
I/OmxPlugin(30096): slice height not available, assuming height
I/OmxPlugin(30096): rotation not available, assuming 0
I/OmxPlugin(30096): width: 384 height: 288 component: OMX.TI.DUCATI1.VIDEO.DECODER format: 2130706688 stride: 384 sliceHeight: 288 rotation: 0
I/OmxPlugin(30096): Unknown video color format: 7f000100
I/OmxPlugin(30096): Unknown video color format: 7f000100
I/OmxPlugin(30096): Unknown video color format: 7f000100


* With kSoftwareCodecsOnly:

I/OmxPlugin(29782): XXX CreateDecoder: aMimeChars="video/mp4"
I/OmxPlugin(29782): Init: MediaExtractor found videoTrackIndex=0, videoMime="video/avc"
I/OmxPlugin(29782): Init: MediaExtractor found audioTrackIndex=1, audioMime="audio/mp4a-latm"
I/OmxPlugin(29782): Init: video duration=185819152 us
I/OmxPlugin(29782): Init: audio duration=185829297 us
I/OmxPlugin(29782): stride not available, assuming width
I/OmxPlugin(29782): slice height not available, assuming height
I/OmxPlugin(29782): rotation not available, assuming 0
I/OmxPlugin(29782): width: 320 height: 240 component: OMX.google.h264.decoder format: 19 stride: 320 sliceHeight: 240 rotation: 0
I/OmxPlugin(29782): channelCount: 2 sampleRate: 44100
I/OmxPlugin(29782): stride not available, assuming width
I/OmxPlugin(29782): slice height not available, assuming height
I/OmxPlugin(29782): rotation not available, assuming 0
I/OmxPlugin(29782): width: 320 height: 192 component: OMX.google.h264.decoder format: 19 stride: 320 sliceHeight: 192 rotation: 0

Comment 3

[Chris Peterson [:cpeterson]]

mediainfo says test.mp4 is 320x178, encoded as H264 Baseline Profile Level 3.0.

Comment 4

[cajbir (:cajbir)]

We don't support color format OMX_TI_COLOR_FormatYUV420PackedSemiPlanar which is 0x7f000100. This will be why no video is displaying. If we add support for that in OmxCodec::ToVideo it should play. I don't have a device to test unfortunately. cpeterson, do you want to implement the color format and test or would you like me to implement it and you test?

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-1-log-omx-errors.patch

Part 1: Add some OmxPlugin error logging.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-2-check-color-format.patch

Part 2: Use software decoder for color formats we don't know how to convert.

This change is ugly because OMXCodec must initialize the videoSource and parse the video's AVCDecoderConfigurationRecord before we can check the color format. In the normal case, we recognize the color format and continue. If we do not know how to convert the color format, throw away the original videoSource and initialize a new one using Stagefright's software decoder.

Do you know of an easier way to determine the video's color format? Or should we not bother jumping through so many hoops? Currently, if we don't recognize a color format, the screen will be blank but audio will play correctly.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: WIP-part-3-convert-YUV420PackedSemiPlanar.patch

Part 3: Add color conversion for OMX_TI_COLOR_FormatYUV420PackedSemiPlanar.

WIP! This patch does not work because I have not figured out how to extract the U and V planes without crashing. I just wanted to share my work with you, in case you know how to fix it. <:)

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson (:cpeterson) from comment #6)
> Do you know of an easier way to determine the video's color format? Or
> should we not bother jumping through so many hoops? Currently, if we don't
> recognize a color format, the screen will be blank but audio will play
> correctly.

doublec: on further thought, silently falling back to the software decoder when we don't recognize the hardware decoder's color format is good for users, but will make our testing more difficult. QA will have a harder time identifying devices that need a new color conversion function to be written (because they will see a slow software-decoded video instead of blank screen).

Comment 9

[cajbir (:cajbir)]

(In reply to Chris Peterson (:cpeterson) from comment #7)
> Created attachment 654496 [details] [diff] [review]
> WIP-part-3-convert-YUV420PackedSemiPlanar.patch
> 
> Part 3: Add color conversion for OMX_TI_COLOR_FormatYUV420PackedSemiPlanar.
> 
> WIP! This patch does not work because I have not figured out how to extract
> the U and V planes without crashing. I just wanted to share my work with
> you, in case you know how to fix it. <:)

Have a look in the Android source at the file ./frameworks/base/media/libstagefright/colorconversion/SoftwareRenderer.cpp in the 'render' function. There's code there that does alignment adjustments on the stride, etc that may be needed.

Comment 10

[Chris Peterson [:cpeterson]]

Patch #1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/542e9ac22e76

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/542e9ac22e76

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: part-3-local-MetaData-pointer.patch

Part 3: Simplify SetVideoFormat() by using local MetaData pointer.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: part-4-rename-conversion-functions.patch

Part 4: Rename color conversion functions to use the standard OMX_COLOR enum names.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: part-5-fix-stride-and-slice-height.patch

Part 5: Stagefright's kKeyWidth and kKeyHeight are actually the stride and slice height.

Comment 16

[Chris Peterson [:cpeterson]]

Attachment: part-6-convert-YUV420PackedSemiPlanar.patch

Part 6: Add color conversion function for OMX_TI_COLOR_FormatYUV420PackedSemiPlanar.

Comment 17

[Chris Peterson [:cpeterson]]

Attachment: part-7-sanity-check-metadata.patch

Part 7: Add sanity checking of video format metadata.

Do you think these error checks are too strict? We may be able to assume reasonable defaults for some unexpected values (e.g. substituting 0,0 for a negative crop rect positions), but we're probably just asking for trouble by trying to play a video with bad metadata.

Comment 19

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d94dbeb794c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be1a01f58f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/092829268463
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c380a0dbf45
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0947175091

Comment 20

[Chris Peterson [:cpeterson]]

Triage, we probably don't need to track this for fennec=16+ because we won't ship Stagefright video until 17.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Going to set tracking 17+ on the assumption that this is on track for 17ish.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d94dbeb794c
https://hg.mozilla.org/mozilla-central/rev/4be1a01f58f3
https://hg.mozilla.org/mozilla-central/rev/092829268463
https://hg.mozilla.org/mozilla-central/rev/0c380a0dbf45
https://hg.mozilla.org/mozilla-central/rev/1d0947175091

Comment 23

[Chris Peterson [:cpeterson]]

Comment on attachment 661893 [details] [diff] [review]
part-3-local-MetaData-pointer.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782508
User impact if declined: Galaxy Nexus users will see a blank screen when using H264 hardware decoder (the default as of bug 782508).
Testing completed (on m-c, etc.): This change has been on m-c for 3 days without any major fallout (other than a fixed build break)
Risk to taking this patch (and alternatives if risky): Medium risk because this patch changes H264 code paths used on all Android and B2G devices.
String or UUID changes made by this patch: N/A

Comment 24

[Chris Peterson [:cpeterson]]

Comment on attachment 661895 [details] [diff] [review]
part-4-rename-conversion-functions.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782508
User impact if declined: Galaxy Nexus users will see a blank screen when using H264 hardware decoder (the default as of bug 782508).
Testing completed (on m-c, etc.): This change has been on m-c for 3 days without any major fallout (other than a fixed build break)
Risk to taking this patch (and alternatives if risky): Medium risk because this patch changes H264 code paths used on all Android and B2G devices.
String or UUID changes made by this patch: N/A

Comment 25

[Chris Peterson [:cpeterson]]

Comment on attachment 661896 [details] [diff] [review]
part-5-fix-stride-and-slice-height.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782508
User impact if declined: Galaxy Nexus users will see a blank screen when using H264 hardware decoder (the default as of bug 782508).
Testing completed (on m-c, etc.): This change has been on m-c for 3 days without any major fallout (other than a fixed build break)
Risk to taking this patch (and alternatives if risky): Medium risk because this patch changes H264 code paths used on all Android and B2G devices.
String or UUID changes made by this patch: N/A

Comment 26

[Chris Peterson [:cpeterson]]

Comment on attachment 661898 [details] [diff] [review]
part-6-convert-YUV420PackedSemiPlanar.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782508
User impact if declined: Galaxy Nexus users will see a blank screen when using H264 hardware decoder (the default as of bug 782508).
Testing completed (on m-c, etc.): This change has been on m-c for 3 days without any major fallout (other than a fixed build break)
Risk to taking this patch (and alternatives if risky): Medium risk because this patch changes H264 code paths used on all Android and B2G devices.
String or UUID changes made by this patch: N/A

Comment 27

[Chris Peterson [:cpeterson]]

Comment on attachment 661899 [details] [diff] [review]
part-7-sanity-check-metadata.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782508
User impact if declined: Galaxy Nexus users will see a blank screen when using H264 hardware decoder (the default as of bug 782508).
Testing completed (on m-c, etc.): This change has been on m-c for 3 days without any major fallout (other than a fixed build break)
Risk to taking this patch (and alternatives if risky): Medium risk because this patch changes H264 code paths used on all Android and B2G devices.
String or UUID changes made by this patch: N/A

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Sounds like we should get some broader testing to ensure this isn't going to surprise us with unexpected behaviour on non-Nexus devices.  Adding qawanted, please test this with several top devices to see if other models exhibit any fallout or strange behaviours.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Setting Jason Smith to the QA contact so we can also test this on B2G devices.

Comment 30

[Jason Smith [:jsmith]]

On the B2G side, I tried out Aaron's test case with the video app. The test case worked with no issues (mp4 video played correctly, no garbleness, etc) on the 9/28 build. There *has* been a regression though with mp3 files recently though (they don't play on the FF OS device right now), although I'm not sure its related to this patch or not (https://github.com/mozilla-b2g/gaia/issues/5466). 

We probably need to figure out if this patch causes the mp3 regression or not.

Comment 31

[Aaron Train [:aaronmt]]

Tested via Nexus 7 and can confirm the fix attached does now work on the aforementioned test-case.

On the Galaxy Note, Asus Transformer TF201, and Samsung Galaxy SII the video is now completely green.

Please post a fix for these devices.

Comment 32

[cajbir (:cajbir)]

(In reply to Aaron Train [:aaronmt] from comment #31)> 
> On the Galaxy Note, Asus Transformer TF201, and Samsung Galaxy SII the video
> is now completely green.
> 
> Please post a fix for these devices.

This is a different bug. Please raise one for that issue. Sounds a lot like bug 786103 though.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Cpeterson - can you confirm if your patch is causing the issues Jason mentions in comment 30?  We'll need to know this before we can approve for Aurora landing. If it's a separate bug, let's get that filed.

Comment 34

[Chris Peterson [:cpeterson]]

(In reply to Aaron Train [:aaronmt] from comment #31)
> Tested via Nexus 7 and can confirm the fix attached does now work on the
> aforementioned test-case.
> 
> On the Galaxy Note, Asus Transformer TF201, and Samsung Galaxy SII the video
> is now completely green.
> 
> Please post a fix for these devices.

aaronmt, this fix is specifically for the Galaxy Nexus. The Nexus 7, Galaxy Note, and Galaxy S II have different chipsets. If those devices have green video, please open a new bug report for each device.

Comment 35

[Chris Peterson [:cpeterson]]

(In reply to Lukas Blakk [:lsblakk] from comment #33)
> Cpeterson - can you confirm if your patch is causing the issues Jason
> mentions in comment 30?  We'll need to know this before we can approve for
> Aurora landing. If it's a separate bug, let's get that filed.

lsblakk, I don't have a B2G device to test, but my patch landed in build 09-21 (comment 22) and the B2G mp3 bug was first reported on 9/28 (bug 795623).

Comment 36

[Chris Peterson [:cpeterson]]

(In reply to Aaron Train [:aaronmt] from comment #31)
> On the Galaxy Note, Asus Transformer TF201, and Samsung Galaxy SII the video
> is now completely green.

aaronmt, did these devices work before my fix in the Nightly 9/21 build?

lsblakk, I'd like to uplift this fix to Aurora 17. AFAICT, my fix did not cause the B2G mp3 bug.

Comment 37

[Aaron Train [:aaronmt]]

Missed the last couple comments here, Chris, I'll file a bug and take a look regarding your comments tomorrow.

As for this bug, this is verified on the GN.

Comment 38

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 661893 [details] [diff] [review]
part-3-local-MetaData-pointer.patch

Thanks for clarifying, Aaron - and for filing bug 797364 to track the green video issue on the other devices. Approving this one for Aurora.

Comment 39

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/51886f0ddb4a
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a96403da4dd
https://hg.mozilla.org/releases/mozilla-aurora/rev/890ca8014c5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/f58f005bddf1
https://hg.mozilla.org/releases/mozilla-aurora/rev/4564131d06d2