Closed Bug 785275 Opened 9 years ago Closed 9 years ago

Galaxy S III hardware decoder shows green bars when playing non-720p videos (OMX_COLOR_FormatYUV420SemiPlanar)

Categories

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

17 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox16 --- unaffected
firefox17 + verified
firefox18 + verified
firefox19 --- verified

People

(Reporter: cpeterson, Assigned: ajones)

References

Details

Attachments

(2 files, 1 obsolete file)

STR:
1. Load http://videojs.com
2. Play video

AR:
The video has green bar artifacts.

Presumably this is a bug in our OMX_COLOR_FormatYUV420SemiPlanar color conversion function, though there are comments in the VLC source code referencing some bugs in the Galaxy S II's hardware decoder (OMX.SEC.avcdec). So it is possibly that the Galaxy S III has similar bugs.
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
The green bars only seem to affect non-720p videos.
Depends on: 786103
Blocks: 782508
Priority: -- → P2
Summary: Galaxy S III video shows green bars when color converting OMX_COLOR_FormatYUV420SemiPlanar → Galaxy S III hardware decoder shows green bars when playing non-720p videos (OMX_COLOR_FormatYUV420SemiPlanar)
Blocks: 787227
No longer blocks: 782508
I can still repro this bug on Galaxy S III.

STR:
1. Load http://people.mozilla.org/~cpeterson/videos/oceans.mp4

ACTUAL RESULT:
The video has ghosting artifacts near the seagulls and a green bar.

EXPECTED RESULT:
The mp4 video should look like the webm video:
http://people.mozilla.org/~cpeterson/videos/oceans.webm
Version: Trunk → 17 Branch
Nick, can you take a look at this?
Assignee: cpeterson → ncameron
Nick, this is probably a bug in OmxPlugin::ToVideoFrame_YUV420SemiPlanar(), but it could be a problem with the hardware decoder misreporting the video's slice height. See:

http://git.videolan.org/gitweb.cgi/vlc.git/?p=vlc.git;a=blob;f=modules/codec/omxil/omxil.c;h=0733648ba652754dea93a4b4fa79c90430cf0a08;hb=HEAD#l515
(In reply to Joe Drew (:JOEDREW! \o/) from comment #3)
> Nick, can you take a look at this?

Not very easily - I don't have an S3 and nor does anyone else in the office. Ajones does though, so do you mind if I kick this in his direction? Otherwise I'm always happy to expense new hardware :-)
In the interests of getting this done soonest, let's let Anthony have this. :)
Assignee: ncameron → ajones
Can we use the stagefright color conversion routines on Android like we are doing on B2G now? See bug 794061 for details. If so, that may fix all our issues here.
Try run for 01371c2ccaad is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=01371c2ccaad
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/ajones@mozilla.com-01371c2ccaad
This fixes bug 793764 on the galaxy note for me.
Oops, wrong bug number. This fixes bug 797364 on the galaxy note for me.
Try run for 6e325ec0f4cf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6e325ec0f4cf
Results (out of 38 total builds):
    success: 34
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6e325ec0f4cf
Comment on attachment 669834 [details] [diff] [review]
Limit the slice height using the calculated buffer height

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

Anthony, have you tested both 720p and non-720p videos? Here are some test videos I use:

https://people.mozilla.com/~cpeterson/videos/

If you have tested different video sizes, then LGTM.

::: media/omx-plugin/OmxPlugin.cpp
@@ +600,5 @@
>                aData, mVideoStride, mVideoWidth/2, mVideoHeight/2, 2, 3);
>  }
>  
>  void OmxDecoder::ToVideoFrame_YUV420SemiPlanar(VideoFrame *aFrame, int64_t aTimeUs, void *aData, size_t aSize, bool aKeyFrame) {
> +  // There is a bug in the OMX.SEC.avc.dec where it returns an implausibly high

s/OMX.SEC.avc.dec/OMX.SEC.avcdec/
Attachment #669834 - Flags: review?(cpeterson) → review+
Anthony, we should uplift your fix all the way to Beta 17. Android's support for H.264 platform decoders in Beta 17 will surely get some press, so fixing green video bugs is a high priority. :)
Blocks: 797364
Try run for 6e325ec0f4cf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6e325ec0f4cf
Results (out of 39 total builds):
    success: 35
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6e325ec0f4cf
(In reply to Chris Peterson (:cpeterson) from comment #14)
> Comment on attachment 669834 [details] [diff] [review]
> Limit the slice height using the calculated buffer height
> 
> Review of attachment 669834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Anthony, have you tested both 720p and non-720p videos?

I tried 720p and there was no issue. I expect this is because the slice height is a multiple of 16. That is 264 appears to have been rounded up to 272 for some odd reason. Similarly 1080p reports slice height of 1088 but plays fine with the patch.
Comment on attachment 670173 [details] [diff] [review]
Limit the slice height using the calculated buffer height v2

LGTM!
Attachment #670173 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/392c04d28358
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Status: RESOLVED → VERIFIED
we'd take an uplift to Beta for this if if comes in the next week or so for an earlier beta build.
Attachment #670173 - Flags: approval-mozilla-beta?
Attachment #670173 - Flags: approval-mozilla-aurora?
Attachment #670173 - Flags: approval-mozilla-beta?
Attachment #670173 - Flags: approval-mozilla-beta+
Attachment #670173 - Flags: approval-mozilla-aurora?
Attachment #670173 - Flags: approval-mozilla-aurora+
QA Contact: aaron.train
Depends on: 802787
No longer depends on: 802787
You need to log in before you can comment on or make changes to this bug.