Bug 29717 - [GTK] missing support for anamorphic PAR video size
Summary: [GTK] missing support for anamorphic PAR video size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-24 09:32 PDT by Philippe Normand
Modified: 2009-10-04 05:26 PDT (History)
5 users (show)

See Also:


Attachments
patch to expose p-a-r from the sink to the player, used during natural size calculation (7.02 KB, patch)
2009-09-28 06:21 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
first patch: clean of caps handling in video sink (2.83 KB, patch)
2009-10-01 05:39 PDT, Philippe Normand
gns: review-
Details | Formatted Diff | Diff
second patch: handle p-a-r in player and sink (6.04 KB, patch)
2009-10-01 05:40 PDT, Philippe Normand
gns: review-
Details | Formatted Diff | Diff
updated patch with styles fixes (6.33 KB, patch)
2009-10-02 00:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
first patch: clean of caps handling in video sink (updated) (2.89 KB, patch)
2009-10-02 02:43 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
first patch: clean of caps handling in video sink (updated) (2.97 KB, patch)
2009-10-02 03:07 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2009-09-24 09:32:08 PDT
The test LayoutTests/media/video-size-intrinsic-scale.html is currently failing. I think it is because  the webkit video sink doesn't handle properly the picture aspect ratio data reported by the upstream gstreamer elements. In fact in VideoSinkGstreamer.cpp, the par_n and par_d don't seem to be used.
Comment 1 Philippe Normand 2009-09-25 09:13:42 PDT
Turns out to be bugs in qtdemux too:

https://bugzilla.gnome.org/show_bug.cgi?id=596326
https://bugzilla.gnome.org/show_bug.cgi?id=596319

And I started working on a patch for our video sink that would properly handle the pixel-aspect-ratio data reported by GStreamer.
Comment 2 Philippe Normand 2009-09-28 01:41:08 PDT
One more! ;)

https://bugzilla.gnome.org/show_bug.cgi?id=596571
Comment 3 Philippe Normand 2009-09-28 06:21:29 PDT
Created attachment 40229 [details]
patch to expose p-a-r from the sink to the player, used during natural size calculation
Comment 4 Philippe Normand 2009-10-01 05:39:35 PDT
Created attachment 40435 [details]
first patch: clean of caps handling in video sink
Comment 5 Philippe Normand 2009-10-01 05:40:12 PDT
Created attachment 40436 [details]
second patch: handle p-a-r in player and sink
Comment 6 Gustavo Noronha (kov) 2009-10-01 08:35:22 PDT
Comment on attachment 40435 [details]
first patch: clean of caps handling in video sink

> @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps)
>      priv->width = width;
>      priv->height = height;
>  
> -    /* We dont yet use fps or pixel aspect into but handy to have */
> -    priv->fps_n = gst_value_get_fraction_numerator(fps);
> -    priv->fps_d = gst_value_get_fraction_denominator(fps);
> +    /* We dont yet use fps but handy to have */
> +    if (!gst_structure_get_fraction(structure, "framerate",
> +                                    &priv->fps_n, &priv->fps_d))
> +        return FALSE;

So, I fail to see why we would return here, because failing to retrieve something we do not even use failed. Does this failing mean we will, say, fail to play the video? Returning here will only cause the pixel aspect ratio to not be set, in case we would be able to fetch it, but not this, so maybe just set a sane default, like you are doing for pixel aspect ratio.
Comment 7 Gustavo Noronha (kov) 2009-10-01 08:48:03 PDT
Comment on attachment 40436 [details]
second patch: handle p-a-r in player and sink

>      if (GstPad* pad = gst_element_get_static_pad(m_videoSink, "sink")) {
> -        gst_video_get_size(GST_PAD(pad), &x, &y);
> +        gst_video_get_size(GST_PAD(pad), &width, &height);
> +        GstCaps* caps = GST_PAD_CAPS(pad);
> +        gfloat par;
> +        gint par_n, par_d;

I think this is a good oportunity to start fixing the style in this code, regarding variable names. I would recommend turning par_n and par_d into pixelAspectRatioNumerator, and pixelAspectRatioDenominator, and par to pixelAspectRatio. We should go this way at least for MediaPlayerPrivate. VideoSink is less problematic, because I believe we will be moving it out of WebKit sometime, but try to follow it there for now.

> +    // re-create the cairo surface only if the size changed

Comments should be full sentences, with capitalization, and punctuation =).

> +    if (size != m_size) {
> +        if (m_surface)
> +            cairo_surface_destroy(m_surface);
> +        m_surface = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, size.width(),
> +                                               size.height());
> +        g_object_set(m_videoSink, "surface", m_surface, 0);
> +    }

So this size is after the scaling has been applied in here, right?:

>      // TODO: We copy the data twice right now. This could be easily improved.
>      cairo_t* cr = cairo_create(priv->surface);
> +    cairo_scale(cr, par, 1.0 / par);
>      cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
>      cairo_set_source_surface(cr, src, 0, 0);
>      cairo_surface_destroy(src);
Comment 8 Philippe Normand 2009-10-01 23:54:32 PDT
(In reply to comment #6)
> (From update of attachment 40435 [details])
> > @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps)
> >      priv->width = width;
> >      priv->height = height;
> >  
> > -    /* We dont yet use fps or pixel aspect into but handy to have */
> > -    priv->fps_n = gst_value_get_fraction_numerator(fps);
> > -    priv->fps_d = gst_value_get_fraction_denominator(fps);
> > +    /* We dont yet use fps but handy to have */
> > +    if (!gst_structure_get_fraction(structure, "framerate",
> > +                                    &priv->fps_n, &priv->fps_d))
> > +        return FALSE;
> 
> So, I fail to see why we would return here, because failing to retrieve
> something we do not even use failed. Does this failing mean we will, say, fail
> to play the video? Returning here will only cause the pixel aspect ratio to not
> be set, in case we would be able to fetch it, but not this, so maybe just set a
> sane default, like you are doing for pixel aspect ratio.

I only kept the previous behavior of the code in this patch...
What would be a "sane default" fps? :)
Comment 9 Philippe Normand 2009-10-02 00:16:34 PDT
Created attachment 40497 [details]
updated patch with styles fixes
Comment 10 Philippe Normand 2009-10-02 00:21:30 PDT
(In reply to comment #6)
> (From update of attachment 40435 [details])
> > @@ -178,14 +172,13 @@ webkit_video_sink_set_caps(GstBaseSink* bsink, GstCaps* caps)
> >      priv->width = width;
> >      priv->height = height;
> >  
> > -    /* We dont yet use fps or pixel aspect into but handy to have */
> > -    priv->fps_n = gst_value_get_fraction_numerator(fps);
> > -    priv->fps_d = gst_value_get_fraction_denominator(fps);
> > +    /* We dont yet use fps but handy to have */
> > +    if (!gst_structure_get_fraction(structure, "framerate",
> > +                                    &priv->fps_n, &priv->fps_d))
> > +        return FALSE;
> 
> So, I fail to see why we would return here, because failing to retrieve
> something we do not even use failed. Does this failing mean we will, say, fail
> to play the video? Returning here will only cause the pixel aspect ratio to not
> be set, in case we would be able to fetch it, but not this, so maybe just set a
> sane default, like you are doing for pixel aspect ratio.

As discussed on IRC with slomo :)

09:13:59 <     slomo> philn-tp: if there's no framerate something is really broken and you shouldn't play the video
09:14:14 <  philn-tp> slomo: ok so the code is fine as it is?
09:14:18 <     slomo> philn-tp: even still images have a framerate (0/1)
09:14:21 < slomo> yes
Comment 11 Gustavo Noronha (kov) 2009-10-02 01:29:08 PDT
(In reply to comment #10)
> As discussed on IRC with slomo :)
> 
> 09:13:59 <     slomo> philn-tp: if there's no framerate something is really
> broken and you shouldn't play the video
> 09:14:14 <  philn-tp> slomo: ok so the code is fine as it is?
> 09:14:18 <     slomo> philn-tp: even still images have a framerate (0/1)
> 09:14:21 < slomo> yes

Very good, thanks! I recommend using g_return_val_if_fail there, then, since this is really one of those "shouldn't happen" situations.
Comment 12 Philippe Normand 2009-10-02 02:43:36 PDT
Created attachment 40501 [details]
first patch: clean of caps handling in video sink (updated)
Comment 13 Philippe Normand 2009-10-02 03:07:38 PDT
Created attachment 40502 [details]
first patch: clean of caps handling in video sink (updated)
Comment 14 Gustavo Noronha (kov) 2009-10-02 04:00:02 PDT
Comment on attachment 40497 [details]
updated patch with styles fixes

Thanks you!
Comment 15 WebKit Commit Bot 2009-10-02 04:15:06 PDT
Comment on attachment 40497 [details]
updated patch with styles fixes

Clearing flags on attachment: 40497

Committed r49019: <http://trac.webkit.org/changeset/49019>
Comment 16 WebKit Commit Bot 2009-10-02 04:15:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Gustavo Noronha (kov) 2009-10-02 04:23:29 PDT
Reopening so that the other patch can be landed by the script.
Comment 18 WebKit Commit Bot 2009-10-02 04:32:47 PDT
Comment on attachment 40502 [details]
first patch: clean of caps handling in video sink (updated)

Clearing flags on attachment: 40502

Committed r49021: <http://trac.webkit.org/changeset/49021>
Comment 19 WebKit Commit Bot 2009-10-02 04:32:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Eric Seidel (no email) 2009-10-02 09:03:09 PDT
Sorry, early closure is tracked by bug 28230.  (Assuming there was some way for the commit-queue to know not to close this?)
Comment 21 Gustavo Noronha (kov) 2009-10-04 05:26:50 PDT
(In reply to comment #20)
> Sorry, early closure is tracked by bug 28230.  (Assuming there was some way for
> the commit-queue to know not to close this?)

Hey, this was caused by the patch not being marked for review, so I don't think it was a problem in the script behavior.