[webkit-reviews] review denied: [Bug 29717] [GTK] missing support for anamorphic PAR video size : [Attachment 40436] second patch: handle p-a-r in player and sink

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 1 08:48:02 PDT 2009


Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 29717: [GTK] missing support for anamorphic PAR video size
https://bugs.webkit.org/show_bug.cgi?id=29717

Attachment 40436: second patch: handle p-a-r in player and sink
https://bugs.webkit.org/attachment.cgi?id=40436&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
>      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);


More information about the webkit-reviews mailing list