Closed Bug 811730 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

19 Branch
ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 + fixed
firefox20 --- fixed
fennec 19+ ---

People

(Reporter: xti, Assigned: nical)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image 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
Component: General → Video/Audio
Product: Firefox for Android → Core
Version: Firefox 19 → 19 Branch
Is this a recent regression? This might be a regression from bug 802787, a Galaxy SII color conversion bug fixed last week.
tracking-fennec: --- → ?
Keywords: qawanted
QA Contact: kbrosnan
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
Assignee: nobody → cpeterson
tracking-fennec: ? → 19+
nsilva, this video bug looks like a regression from OMTC bug 773440.
Assignee: cpeterson → nical.bugzilla
Keywords: regression
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)?
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.
Summary: HTML5 videos are played in black and white mode (and some pink traces) → HTML5 videos are played in black and white mode (and some pink traces) Galaxy S II
Duplicate of this bug: 817320
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.
Attachment #687876 - Flags: review?(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/nsilva@mozilla.com-d0030a1d853a/try-android/fennec-20.0a1.en-US.android-arm.apk
Status: NEW → ASSIGNED
Keywords: qawanted
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. :)
Attachment #687876 - Flags: review?(cpeterson) → review-
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).
Attachment #687876 - Attachment is obsolete: true
Attachment #688195 - Flags: review?(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.
Attachment #688195 - Flags: review?(cpeterson) → review+
> Status: ASSIGNED → NEW 
wat?
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/aa649086ab5b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Please nominate for uplift to Aurora if this is low-risk and ready.
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:
Attachment #688195 - Flags: approval-mozilla-aurora?
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.
Attachment #688195 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.