811730 - HTML5 videos are played in black and white mode (and some pink traces) Galaxy S II

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Cristian Nicolae (:xti)]

Attachment: screenshot

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

Steps to reproduce:
1. Go to metacafe.com
2. Play any video

Expected result:
The selected video is played correctly.

Actual result:
The video is played in black and white. Some pink traces can be noticed too

Note:
This issue is reproducible just on the Nightly

Comment 1

[Chris Peterson [:cpeterson]]

Is this a recent regression? This might be a regression from bug 802787, a Galaxy SII color conversion bug fixed last week.

Comment 2

[Cristian Nicolae (:xti)]

This issue is not reproducible on 11/06, but occurs on 11/07.

I narrowed down the regression using the inbound tinderbox builds:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=60c78a559a84&tochange=35ba50a6a97e

Comment 3

[Chris Peterson [:cpeterson]]

nsilva, this video bug looks like a regression from OMTC bug 773440.

Comment 4

[Nicolas Silva [:nical]]

I think I know what's going on here.

To be sure, what are the pixel formats we can receive from the decoder?
In particular, can we have have Cb and Cr buffers with different skips, and interleaved in specific ways (like less than 8bits per channels)?

Comment 5

[Chris Peterson [:cpeterson]]

The Galaxy S II decoder returns OMX_COLOR_FormatYUV420SemiPlanar data, so it should not have different Cb and Cr skips. Android's OmxPlugin.cpp sets up the VideoFrame for YUV420SemiPlanar data here:

https://hg.mozilla.org/mozilla-central/file/d2fbc67f69f5/media/omx-plugin/OmxPlugin.cpp#l603

Note that the Galaxy S II decoder has known "quirks" related to frame stride and height.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Take skip into account when copying frames into shared memory.

When I implemented ShmemYCbCrImage I forgot to take the skip into account when copying frames. I think the problem comes from there. I added the necessary code, but for some reason I am having troubles getting adb to work these days and I'm extremely busy with another bug.
I pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=d0030a1d853a

If we're in a hurry to fix this, can someone test my patch on an android device? Otherwise I will receive my work machine this week and install a more stable environment and test it.

Comment 8

[Chris Peterson [:cpeterson]]

QA: Does anyone with a Galaxy S II have time to test Nicolas' build? I don't have my Galaxy S II today, but I will be able to test tomorrow.

http://ftp-scl3.mozilla.com/pub/mozilla.org/firefox/try-builds/[e-mail]-d0030a1d853a/try-android/fennec-20.0a1.en-US.android-arm.apk

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 687876 [details] [diff] [review]
Take skip into account when copying frames into shared memory.

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

Looking good, but CopyLineWithSkip() does not actually copy data from src to dst! <:)

::: gfx/layers/ipc/ShmemYCbCrImage.cpp
@@ +149,5 @@
>    }
>    return true;
>  }
>  
> +static void CopyLineWithSkip(uint8_t* src, uint8_t* dst, uint32_t len, uint32_t skip) {

The src parameter should be a const pointer.

@@ +151,5 @@
>  }
>  
> +static void CopyLineWithSkip(uint8_t* src, uint8_t* dst, uint32_t len, uint32_t skip) {
> +  for (uint32_t i = 0; i < len; ++i) {
> +    dst = src;

Did you mean `*dst = src;`?

@@ +161,1 @@
>  bool ShmemYCbCrImage::CopyData(uint8_t* aYData, uint8_t* aCbData, uint8_t* aCrData,

Can these aYData, aCbData, and aCrData parameters be const pointers?

@@ +187,5 @@
> +             aCbData + i * aCbCrStride,
> +             aCbCrSize.width);  
> +      memcpy(GetCrData() + i * GetCbCrStride(),
> +             aCrData + i * aCbCrStride,
> +             aCbCrSize.width);  

Some trailing whitespace snuck into your patch. :)

Comment 10

[Nicolas Silva [:nical]]

Attachment: Take skip into account when copying frames into shared memory.

Wow, I deserve some kind of trophy of shame or open badge for that previous patch.
This version has been fixed after consumption of coffee (not that it offers so much guaranties).

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 688195 [details] [diff] [review]
Take skip into account when copying frames into shared memory.

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

LGTM! I was unable to test your patch on my Galaxy S II because it is running Gingerbread. However, I could reproduce the bug on my Galaxy Nexus running Jelly Bean 4.2 and your patch *did* fix the bug there.

Comment 12

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa649086ab5b

Comment 13

[Nicolas Silva [:nical]]

> Status: ASSIGNED → NEW 
wat?

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift to Aurora if this is low-risk and ready.

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 688195 [details] [diff] [review]
Take skip into account when copying frames into shared memory.

[Approval Request Comment]
Bug caused by (feature/regressing bug #773440): 
User impact if declined: Video displayed improperly with
Testing completed (on m-c, etc.): Landed 2012-12-05
Risk to taking this patch (and alternatives if risky):
String or UUID changes made by this patch:

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 688195 [details] [diff] [review]
Take skip into account when copying frames into shared memory.

We're going to assume that this nomination is low risk, please only land if that's the case.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e511f4bbab30