Bug 29717 – [GTK] missing support for anamorphic PAR video size

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

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.

[Philippe Normand]

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.

[Philippe Normand]

One more! ;)

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

[Philippe Normand]

Created attachment 40229 [details]
patch to expose p-a-r from the sink to the player, used during natural size calculation

[Philippe Normand]

Created attachment 40435 [details]
first patch: clean of caps handling in video sink

[Philippe Normand]

Created attachment 40436 [details]
second patch: handle p-a-r in player and sink

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

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);

[Philippe Normand]

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

[Philippe Normand]

Created attachment 40497 [details]
updated patch with styles fixes

[Philippe Normand]

(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

[Gustavo Noronha (kov)]

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

[Philippe Normand]

Created attachment 40501 [details]
first patch: clean of caps handling in video sink (updated)

[Philippe Normand]

Created attachment 40502 [details]
first patch: clean of caps handling in video sink (updated)

[Gustavo Noronha (kov)]

Comment on attachment 40497 [details]
updated patch with styles fixes

Thanks you!

[WebKit Commit Bot]

Comment on attachment 40497 [details]
updated patch with styles fixes

Clearing flags on attachment: 40497

Committed r49019: <http://trac.webkit.org/changeset/49019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

Reopening so that the other patch can be landed by the script.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Sorry, early closure is tracked by bug 28230.  (Assuming there was some way for the commit-queue to know not to close this?)

[Gustavo Noronha (kov)]

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