802787 - Mixed video playback results on all channels (H.264, High Profile AVC1.64001E) on the Samsung Galaxy SII

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Samsung Galaxy SII (H.264 Playback) - mozilla-aurora / mozilla-beta

Currently with an H.264, High Profile AVC1.64001E video I am getting mixed results with hardware decoding during playback on my Samsung Galaxy SII (Android 4.0.4).

On mozilla-central (Nightly) [*No playback at all*]→ I/OmxPlugin( 4695): mVideoSource ERROR 0x80000000

On mozilla-beta (Beta) → see attached screenshot

On mozilla-aurora (Aurora) → see attached screenshot

Simply load the test-file URL in the browser.

--
Samsung Galaxy SII (Android 4.0.4)
Nightly (08/14

Comment 1

[Chris Peterson [:cpeterson]]

This bug may be fallout from Galaxy S III bug 785275, which clamped the video stride.

Comment 2

[cajbir (:cajbir)]

I backed out the patch from bug 785275 and the video is the same as the screenshot, except it has a green bar at the bottom, so I don't think that bug affected it.

I note that http://cd.pn/b2 still plays fine, but the test URL doesn't so it must have something different that we are handling incorrectly.

Comment 3

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

Looks like the same problem as 785275 but horizontal instead of vertical. The width in this case is 852 which isn't a multiple of 16. It seems that both the stride and slice height are being reported as the next multiple of 16 up from what they should be.

Comment 4

[cajbir (:cajbir)]

Can you investigate if this can be fixed using Android's ColorConversion class as mentioned in bug 794061 comment 3. I briefly tried using it in bug 803394 and we can use it from Android. If so, this could solve a lot of our color conversion issues.

Comment 5

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

Attachment: Work around misreported stride

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 674834 [details] [diff] [review]
Work around misreported stride

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

::: media/omx-plugin/OmxPlugin.cpp
@@ +606,4 @@
>  
> +  // OMX.SEC.avcdec rounds mVideoStride and mVideoSliceHeight up to the nearest
> +  // multiple of 16 but the data itself is too small to fit. What we do is check
> +  // to see if the video size patches the raw width and height. If so we can

s/patches/matches/?

@@ +610,5 @@
> +  // use those figures instead.
> +
> +  if (aSize * 2 / 3 == mVideoWidth * mVideoHeight) {
> +    videoStride = mVideoWidth;
> +    videoSliceHeight = mVideoHeight;

When the video size matches, what are the values of mVideoStride and mVideoSliceHeight? Are they rounded up to a multiple of 16 that is larger than the video's actual stride or slice height?

The video dimensions are not going to change during playback, but ToVideoFrame_YUV420SemiPlanar() is called every frame. It might make sense to fix up the stride and slice height when they are initialized in OmxDecoder::SetVideoFormat().

Comment 7

[Mozilla RelEng Bot]

Try run for f95e2371b6d1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f95e2371b6d1
Results (out of 1 total builds):
    success: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f95e2371b6d1

Comment 8

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

(In reply to Chris Peterson (:cpeterson) from comment #6)
> Comment on attachment 674834 [details] [diff] [review]
> Work around misreported stride
> @@ +610,5 @@
> > +  // use those figures instead.
> > +
> > +  if (aSize * 2 / 3 == mVideoWidth * mVideoHeight) {
> > +    videoStride = mVideoWidth;
> > +    videoSliceHeight = mVideoHeight;
> 
> When the video size matches, what are the values of mVideoStride and
> mVideoSliceHeight? Are they rounded up to a multiple of 16 that is larger
> than the video's actual stride or slice height?

Yes. That seems to be the case.

> The video dimensions are not going to change during playback, but
> ToVideoFrame_YUV420SemiPlanar() is called every frame. It might make sense
> to fix up the stride and slice height when they are initialized in
> OmxDecoder::SetVideoFormat().

OmxDecoder doesn't have the data size information. Also most of the work in ToVideoFrame_YUV420SemiPlanar is recalculating something that doesn't change.

There is an artefact in the bottom right corner which I still haven't figured out. I'm wondering if there is a better place to fix the issue. The video plays OK in VLC.

Comment 9

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

Attachment: Samsung Galaxy SIII with patch 674834 applied.

Comment 10

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

Attachment: Work around misreported stride v2

Comment 11

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

Created bug 807561 for the green corner problem which appears to have a separate cause.

Comment 13

[Chris Peterson [:cpeterson]]

Comment on attachment 677279 [details] [diff] [review]
Work around misreported stride v2

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

oh, that is tricky! LGTM.

::: media/omx-plugin/OmxPlugin.cpp
@@ +608,5 @@
> +  // multiple of 16 but the data itself is too small to fit. What we do is check
> +  // to see if the video size patches the raw width and height. If so we can
> +  // use those figures instead.
> +
> +  if (aSize == mVideoWidth * mVideoHeight * 3 / 2) {

Are there unusual video dimensions that might still lose precision when divided by 2? Maybe multiplying aSize by 2 might be safer, given the rounding problems you've found with this decoder:

  if (aSize * 2 == mVideoWidth * mVideoHeight * 3) {

Comment 14

[Chris Peterson [:cpeterson]]

This is not a [swdecoder] bug.

Comment 15

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

(In reply to Chris Peterson (:cpeterson) from comment #13)
> Are there unusual video dimensions that might still lose precision when
> divided by 2? Maybe multiplying aSize by 2 might be safer, given the
> rounding problems you've found with this decoder:
> 
>   if (aSize * 2 == mVideoWidth * mVideoHeight * 3) {

A useful example turns out to be:
777 * 333 * 3 / 2 = 258741 * 3 / 2 = 388111.5

In this instance aSize is being reported as 388111. The precision loss in my equality is deliberate because otherwise it would not satisfy the "safer" inequality from the previous patch.

Odd sizes in YUV420 bring up some interesting edge cases in YUV420 when you consider the colour information for the last column or last row of pixels.

Comment 16

[Mozilla RelEng Bot]

Try run for b32c095ed230 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b32c095ed230
Results (out of 15 total builds):
    success: 14
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b32c095ed230

Comment 17

[Mozilla RelEng Bot]

Try run for b32c095ed230 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b32c095ed230
Results (out of 16 total builds):
    success: 14
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b32c095ed230

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b5e96686b9b

Possible to test this?

Comment 19

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

Firstly this problem won't show up on our panda boards. However it would be useful to have a test suite for creating white lists and black lists. All the video tests for webm and ogg are currently disabled for Android so I will look into getting these running and porting them to h264.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b5e96686b9b

Comment 21

[Cristian Nicolae (:xti)]

I cannot reproduce this issue on the latest Nightly build. Closing bug as verified fixed on:

Firefox 19.0a1 (2012-11-15)
Device: Galaxy S2
OS: Android 4.0.3