[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