[webkit-reviews] review denied: [Bug 29717] [GTK] missing support for anamorphic PAR video size : [Attachment 40435] first patch: clean of caps handling in video sink

[bugzilla-daemon at webkit.org]

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 40435: first patch: clean of caps handling in video sink
https://bugs.webkit.org/attachment.cgi?id=40435&action=review

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