976510 - GStreamerReader needs to check display size for aspect-ratio change video

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Use nightly build Firefox and play the testing file, 
The video tag's screen size is wrong.
ref: 975978 OmxDecoder needs to check display size for aspect-ratio change video

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: pixel_aspect_ratio.mp4

testing file.

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: aspect_ratio_omx

I've tried it on both gstreamer 1.0 and 0.10. Testcase is the same one in bug 975978.

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: gstreamer_aspect_ratio

Sorry, uploaded the wrong patch.

Comment 4

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

Comment on attachment 8385053 [details] [diff] [review]
gstreamer_aspect_ratio

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

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1011,5 @@
>  #endif
> +
> +  // Calculate display size according to pixel aspect ratio.
> +  IntSize displaySize;
> +  displaySize = gfx::IntSize(mPicture.width, mPicture.height);

I think you should use ScaleDisplayByAspectRatio() and then IsValidVideoRegion(), like we do in other backends. For example: 

http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFReader.cpp#318
http://mxr.mozilla.org/mozilla-central/source/content/media/plugins/MediaPluginReader.cpp#77

Comment 5

[Alfredo Yang (:alfredo)]

(In reply to Chris Pearce (:cpearce) from comment #4)
> Comment on attachment 8385053 [details] [diff] [review]
> gstreamer_aspect_ratio
> 
> Review of attachment 8385053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gstreamer/GStreamerReader.cpp
> @@ +1011,5 @@
> >  #endif
> > +
> > +  // Calculate display size according to pixel aspect ratio.
> > +  IntSize displaySize;
> > +  displaySize = gfx::IntSize(mPicture.width, mPicture.height);
> 
> I think you should use ScaleDisplayByAspectRatio() and then
> IsValidVideoRegion(), like we do in other backends. For example: 
> 
> http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMFReader.
> cpp#318
> http://mxr.mozilla.org/mozilla-central/source/content/media/plugins/
> MediaPluginReader.cpp#77

Thanks for suggestion. I'll fix it.

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: gstreamer_aspect_ratio

Comment 7

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

Comment on attachment 8385149 [details] [diff] [review]
gstreamer_aspect_ratio

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

r=cpearce with the comments addressed.

Edwin should double check this, he knows more about gstreamer than I.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1007,4 @@
>  #else
>    GstCaps* caps = gst_pad_get_negotiated_caps(sinkpad);
>    gst_video_format_parse_caps(caps, &mFormat, &mPicture.width, &mPicture.height);
> +  gst_video_parse_caps_pixel_aspect_ratio(caps, &PARNumerator, &PARDenominator);

gst_video_parse_caps_pixel_aspect_ratio() can fail, and if so does it set PARNumerator and PARDenominator? If not, you'll be using PARNumerator and PARDenominator uninitialized.

gst_video_parse_caps_pixel_aspect_ratio() returns false when it fails:
http://www.freedesktop.org/software/gstreamer-sdk/data/docs/2012.5/gst-plugins-base-libs-0.10/gst-plugins-base-libs-gstvideo.html#gst-video-parse-caps-pixel-aspect-ratio

I think you should assume that the aspect ratio is 1 on failure. Other backends stop playback when this happens, but I think we could safely assume a PAR of 1 instead.

@@ +1023,5 @@
> +    gst_structure_get_fraction(structure, "framerate", &fpsNum, &fpsDen);
> +    mInfo.mVideo.mDisplay = ThebesIntSize(displaySize.ToIntSize());
> +    mInfo.mVideo.mHasVideo = true;
> +  } else {
> +    NS_ASSERTION(false, "invalid video display region");

Please call Eos() to stop playback of all streams on error.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8385149 [details] [diff] [review]
gstreamer_aspect_ratio

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

LGTM, modulo Chris' comments.

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: gstreamer_aspect_ratio

Carry r+. Testcase is the same one in bug 975978.
https://tbpl.mozilla.org/?tree=Try&rev=3c46478c4823

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6031a453ef68

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/6031a453ef68