[webkit-reviews] review denied: [Bug 30308] [GStreamer] Add direct support for ARGB videos : [Attachment 41094] 0001-Add-support-for-ARGB-videos.patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Sebastian Dröge
<slomo at circular-chaos.org>'s request for review:
Bug 30308: [GStreamer] Add direct support for ARGB videos
https://bugs.webkit.org/show_bug.cgi?id=30308

Attachment 41094: 0001-Add-support-for-ARGB-videos.patch
https://bugs.webkit.org/attachment.cgi?id=41094&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 
> +    // Calculate the display width/height from the storage width/height and
the pixel aspect ratio
>      displayWidth *= doublePixelAspectRatioNumerator /
doublePixelAspectRatioDenominator;
>      displayHeight *= doublePixelAspectRatioDenominator /
doublePixelAspectRatioNumerator;
>  
> -    scale = MIN (rect.width () / displayWidth, rect.height () /
displayHeight);
> +    // Calculate the largest scale factor that would fill the target surface

> +    scale = MIN(rect.width() / displayWidth, rect.height() / displayHeight);

> +    // And calculate the new display width/height
>      displayWidth *= scale;
>      displayHeight *= scale;
>  
> -    // Calculate gap between border an picture
> +    // Calculate gap between border an picture on every side
>      gapWidth = (rect.width() - displayWidth) / 2.0;
>      gapHeight = (rect.height() - displayHeight) / 2.0;
>  
> -    // paint the rectangle on the context and draw the surface inside.
> +    // paint the rectangle on the context and draw the buffer inside

You may want to make this a full sentence.

> +    // Go to the new origin and center the video frame.
>      cairo_translate(cr, rect.x() + gapWidth, rect.y() + gapHeight);
>      cairo_rectangle(cr, 0, 0, rect.width(), rect.height());
> +    // Scale the video frame according to the pixel aspect ratio.
>      cairo_scale(cr, doublePixelAspectRatioNumerator /
doublePixelAspectRatioDenominator,
>		   doublePixelAspectRatioDenominator /
doublePixelAspectRatioNumerator);
> +    // Scale the video frame to fill the target surface as good as possible.

>      cairo_scale(cr, scale, scale);
> +    // And paint it.
>      cairo_set_source_surface(cr, src, 0, 0);
>      cairo_fill(cr);
>      cairo_restore(cr);

I like the comments, and style fix, but I think they deserve a separate commit.


>      // Use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations.
>      priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0,
>					     webkit_video_sink_timeout_func,
>					     gst_object_ref(sink),
> -					     (GDestroyNotify)gst_object_unref);

> +					     (GDestroyNotify)
gst_object_unref);

rogue space addition. Fix these, and split the patches, and they look good to
go.