Chris Peterson [:cpeterson] Reporter
	Description • 12 years ago

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.

	Chris Peterson [:cpeterson] Reporter
	Comment 1 • 12 years ago

The green bars only seem to affect non-720p videos.

	Chris Peterson [:cpeterson] Reporter
	Comment 2 • 12 years ago

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

	Joe Drew (not getting mail)
	Comment 3 • 12 years ago

Nick, can you take a look at this?

	Chris Peterson [:cpeterson] Reporter
	Comment 4 • 12 years ago

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

	Nick Cameron [:nrc]
	Comment 5 • 12 years ago

(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 :-)

	Joe Drew (not getting mail)
	Comment 6 • 12 years ago

In the interests of getting this done soonest, let's let Anthony have this. :)

	cajbir (:cajbir)
	Comment 7 • 12 years ago

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.

	Mozilla RelEng Bot
	Comment 10 • 12 years ago

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

	cajbir (:cajbir)
	Comment 11 • 12 years ago

This fixes bug 793764 on the galaxy note for me.

	cajbir (:cajbir)
	Comment 12 • 12 years ago

Oops, wrong bug number. This fixes bug 797364 on the galaxy note for me.

	Mozilla RelEng Bot
	Comment 13 • 12 years ago

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

	Chris Peterson [:cpeterson] Reporter
	Comment 14 • 12 years ago

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/

	Chris Peterson [:cpeterson] Reporter
	Comment 15 • 12 years ago

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. :)

	Mozilla RelEng Bot
	Comment 16 • 12 years ago

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

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 17 • 12 years ago

(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.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e) Assignee
	Comment 18 • 12 years ago

removed extra .

	Chris Peterson [:cpeterson] Reporter
	Comment 19 • 12 years ago

Comment on attachment 670173 [details] [diff] [review]
Limit the slice height using the calculated buffer height v2

LGTM!

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/392c04d28358

Should this have a test?

	Ed Morley [:emorley]
	Comment 21 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/392c04d28358

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 22 • 12 years ago

we'd take an uplift to Beta for this if if comes in the next week or so for an earlier beta build.

	Chris Peterson [:cpeterson] Reporter
	Comment 23 • 12 years ago

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

	Chris Peterson [:cpeterson] Reporter
	Comment 24 • 12 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/56324aae3c3d