975978 - OmxDecoder needs to check display size for aspect-ratio change video

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Alfredo Yang (:alfredo)]

Test url: http://people.mozilla.org/~ayang/mp4/aspect_mp4.html

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: aspect_ratio_omx

Comment 2

[Alfredo Yang (:alfredo)]

gstreamer backend also has the same problem.

Comment 3

[Alfredo Yang (:alfredo)]

Comment on attachment 8380537 [details] [diff] [review]
aspect_ratio_omx

Hi Benjamin, could you please check if this patch is ok for RTSP streaming? Thanks.

Comment 4

[Benjamin Chen [:bechen]]

(In reply to Alfredo Yang from comment #3)
> Comment on attachment 8380537 [details] [diff] [review]
> aspect_ratio_omx
> 
> Hi Benjamin, could you please check if this patch is ok for RTSP streaming?
> Thanks.

It looks fine if there is default value for display width/height.

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Comment 6

[u459114]

Alfredo,
Does this problem only exist in OMX decoding pipeline? How about other platform decoder?

Comment 7

[Alfredo Yang (:alfredo)]

(In reply to C.J. Ku[:CJKu] from comment #6)
> Alfredo,
> Does this problem only exist in OMX decoding pipeline? How about other
> platform decoder?

gstreamer backend also has the same problem.

Comment 8

[Alfredo Yang (:alfredo)]

Attachment: aspect_ratio_omx

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8381174 [details] [diff] [review]
aspect_test

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

r=cpearce with changes addressed below.

::: content/media/test/mochitest.ini
@@ +185,5 @@
>    wavedata_u8.wav
>  
>  [test_access_control.html]
> +[test_aspectratio_mp4.html]
> +run-if = toolkit == "gonk"

This test should pass on all platforms that support MP4 right? It may not work everywhere yet, but we should fix that. I think it should pass on Windows at least, can you make this test also run on Windows, and on any other platforms on which it passes, and file bugs for the platforms that don't pass. Thanks!

::: content/media/test/test_aspectratio_mp4.html
@@ +17,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +var v = document.createElement("video"), resource = getPlayableVideo(gMP4AspectRatioTests);

I think you can link to this directly, rather than adding another set of videos (gMP4AspectRatioTests) to manifest.js. Unless you intend to add more files to the test test, other than pixel_aspect_ratio.mp4?

@@ +19,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +var v = document.createElement("video"), resource = getPlayableVideo(gMP4AspectRatioTests);
> +
> +function bodyLoaded(){

You can do this in an "loadedmetadata" event handler.

@@ +26,5 @@
> +  SimpleTest.finish();
> +}
> +
> +if (resource) {
> +  v.src = "http://mochi.test:8888/tests/content/media/test/" + resource.name;

you should not need the "http://mochi.test:8888/tests/content/media/test/" here.

Comment 11

[Alfredo Yang (:alfredo)]

(In reply to Chris Pearce (:cpearce) from comment #10)
> Comment on attachment 8381174 [details] [diff] [review]
> aspect_test
> 
> Review of attachment 8381174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=cpearce with changes addressed below.
> 
> ::: content/media/test/mochitest.ini
> @@ +185,5 @@
> >    wavedata_u8.wav
> >  
> >  [test_access_control.html]
> > +[test_aspectratio_mp4.html]
> > +run-if = toolkit == "gonk"
> 
> This test should pass on all platforms that support MP4 right? It may not
> work everywhere yet, but we should fix that. I think it should pass on
> Windows at least, can you make this test also run on Windows, and on any
> other platforms on which it passes, and file bugs for the platforms that
> don't pass. Thanks!
> 

WMF backend is good. Gstreamer is not good, Randy has filled a bug 976510 for it.
Firefox on Mac seems not support MP4 so far?

> ::: content/media/test/test_aspectratio_mp4.html
> @@ +17,5 @@
> > +<script class="testbody" type="text/javascript">
> > +
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +var v = document.createElement("video"), resource = getPlayableVideo(gMP4AspectRatioTests);
> 
> I think you can link to this directly, rather than adding another set of
> videos (gMP4AspectRatioTests) to manifest.js. Unless you intend to add more
> files to the test test, other than pixel_aspect_ratio.mp4?
> 

Fixed.

> @@ +19,5 @@
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +var v = document.createElement("video"), resource = getPlayableVideo(gMP4AspectRatioTests);
> > +
> > +function bodyLoaded(){
> 
> You can do this in an "loadedmetadata" event handler.
> 

Fixed.

> @@ +26,5 @@
> > +  SimpleTest.finish();
> > +}
> > +
> > +if (resource) {
> > +  v.src = "http://mochi.test:8888/tests/content/media/test/" + resource.name;
> 
> you should not need the "http://mochi.test:8888/tests/content/media/test/"
> here.

Fixed.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Alfredo Yang from comment #11)
> Firefox on Mac seems not support MP4 so far?

Correct, Firefox on Mac does not yet support MP4.

Comment 13

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Comment 14

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Rebase carry r+.

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Carry r+.
https://tbpl.mozilla.org/?tree=Try&rev=43542ecaf578

Comment 17

[Alfredo Yang (:alfredo)]

Attachment: aspect_test

Rebase, carry r+.
https://tbpl.mozilla.org/?tree=Try&rev=c38866a4f04a

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e3e3c262bfa3
https://hg.mozilla.org/integration/b2g-inbound/rev/8196bd736d67

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e3e3c262bfa3
https://hg.mozilla.org/mozilla-central/rev/8196bd736d67