Closed Bug 784329 Opened 12 years ago Closed 12 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 --- verified
firefox18 --- verified
fennec 17+ ---

People

(Reporter: aaronmt, Assigned: cpeterson)

References

()

Details

(Keywords: regression)

Attachments

(7 files, 1 obsolete file)

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
This bug is likely fallout from bug 782508.
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
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
mediainfo says test.mp4 is 320x178, encoded as H264 Baseline Profile Level 3.0.
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?
Attached patch part-1-log-omx-errors.patch — — Splinter Review
Part 1: Add some OmxPlugin error logging.
Attachment #654494 - Flags: review?(chris.double)
Attached patch part-2-check-color-format.patch — — Splinter Review
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.
Attachment #654495 - Flags: review?(chris.double)
Attached patch WIP-part-3-convert-YUV420PackedSemiPlanar.patch (obsolete) — — Splinter Review
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. <:)
Attachment #654496 - Flags: feedback?(chris.double)
Priority: -- → P1
tracking-fennec: ? → 16+
(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).
tracking-fennec: 16+ → ?
tracking-fennec: ? → 16+
Attachment #654494 - Flags: review?(chris.double) → review+
(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.
Attachment #654495 - Flags: review?(chris.double) → review+
Attachment #654496 - Flags: feedback?(chris.double)
Blocks: 787226
No longer blocks: 759945
Whiteboard: [leave open] → [leave open], [swdecoder]
Summary: No video playback with OMX.TI.DUCATI1.VIDEO.DECODER via MP4 in Android → Stagefright: Galaxy Nexus hardware decoder video is blank. Need OMX_TI_COLOR_FormatYUV420PackedSemiPlanar color conversion.
Whiteboard: [leave open], [swdecoder] → [leave open],
Part 3: Simplify SetVideoFormat() by using local MetaData pointer.
Attachment #654496 - Attachment is obsolete: true
Attachment #661893 - Flags: review?(chris.double)
Part 4: Rename color conversion functions to use the standard OMX_COLOR enum names.
Attachment #661895 - Flags: review?(chris.double)
Part 5: Stagefright's kKeyWidth and kKeyHeight are actually the stride and slice height.
Attachment #661896 - Flags: review?(chris.double)
Part 6: Add color conversion function for OMX_TI_COLOR_FormatYUV420PackedSemiPlanar.
Attachment #661898 - Flags: review?(chris.double)
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.
Attachment #661899 - Flags: review?(chris.double)
Attachment #661893 - Flags: review?(chris.double) → review+
Attachment #661895 - Flags: review?(chris.double) → review+
Attachment #661896 - Flags: review?(chris.double) → review+
Attachment #661898 - Flags: review?(chris.double) → review+
Attachment #661899 - Flags: review?(chris.double) → review+
Triage, we probably don't need to track this for fennec=16+ because we won't ship Stagefright video until 17.
tracking-fennec: 16+ → ?
Going to set tracking 17+ on the assumption that this is on track for 17ish.
tracking-fennec: ? → 17+
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
Attachment #661893 - Flags: approval-mozilla-aurora?
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
Attachment #661895 - Flags: approval-mozilla-aurora?
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
Attachment #661896 - Flags: approval-mozilla-aurora?
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
Attachment #661898 - Flags: approval-mozilla-aurora?
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
Attachment #661899 - Flags: approval-mozilla-aurora?
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.
Keywords: qawanted, verifyme
QA Contact: jsmith
Setting Jason Smith to the QA contact so we can also test this on B2G devices.
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.
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.
QA Contact: jsmith → nobody
(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.
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.
(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.
(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).
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [leave open],
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.
Status: RESOLVED → VERIFIED
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.
Attachment #661893 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #661895 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #661896 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #661898 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #661899 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: