Closed Bug 976510 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: rlin, Assigned: ayang)

Details

Attachments

(2 files, 3 obsolete files)

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
Attached video pixel_aspect_ratio.mp4
testing file.
Attached patch aspect_ratio_omx (obsolete) — Splinter Review
I've tried it on both gstreamer 1.0 and 0.10. Testcase is the same one in bug 975978.
Attachment #8384503 - Flags: review?(cpearce)
Attached patch gstreamer_aspect_ratio (obsolete) — Splinter Review
Sorry, uploaded the wrong patch.
Attachment #8384503 - Attachment is obsolete: true
Attachment #8384503 - Flags: review?(cpearce)
Attachment #8385053 - Flags: review?(cpearce)
Attachment #8385053 - Attachment is patch: true
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
Attachment #8385053 - Flags: review?(cpearce) → review-
Assignee: nobody → ayang
(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.
Attached patch gstreamer_aspect_ratio (obsolete) — Splinter Review
Attachment #8385053 - Attachment is obsolete: true
Attachment #8385149 - Flags: review?(cpearce)
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.
Attachment #8385149 - Flags: review?(edwin)
Attachment #8385149 - Flags: review?(cpearce)
Attachment #8385149 - Flags: review+
Comment on attachment 8385149 [details] [diff] [review]
gstreamer_aspect_ratio

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

LGTM, modulo Chris' comments.
Attachment #8385149 - Flags: review?(edwin) → review+
Carry r+. Testcase is the same one in bug 975978.
https://tbpl.mozilla.org/?tree=Try&rev=3c46478c4823
Attachment #8385149 - Attachment is obsolete: true
Attachment #8385892 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6031a453ef68
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.