[webkit-reviews] review denied: [Bug 29997] [GStreamer] Prevent double-memcpy in GStreamer media player : [Attachment 41024] double-memcpy.diff

[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 29997: [GStreamer] Prevent double-memcpy in GStreamer media player
https://bugs.webkit.org/show_bug.cgi?id=29997

Attachment 41024: double-memcpy.diff
https://bugs.webkit.org/attachment.cgi?id=41024&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
First, some content:

 664	 cairo_surface_t* src =
cairo_image_surface_create_for_data(GST_BUFFER_DATA(m_buffer),
 665								   
CAIRO_FORMAT_RGB24,
 666								    width,
height,
 667								    4 * width);


We used to have a surface with CAIRO_FORMAT_ARGB32. Keeping in mind that webkit
does transforms, and should be able to apply effects such as transparency to
the video, are we not loosing some of our current capabilities in changing it
to CAIRO_FORMAT_RGB24?

> -static void mediaPlayerPrivateRepaintCallback(WebKitVideoSink*,
MediaPlayerPrivate* playerPrivate)
> +void mediaPlayerPrivateRepaintCallback(WebKitVideoSink*, GstBuffer *buffer,
MediaPlayerPrivate* playerPrivate)
>  {
> +    g_return_if_fail (GST_IS_BUFFER(buffer));
> +    gst_buffer_replace (&playerPrivate->m_buffer, buffer);

Spaces before brackets here. There are some more of these throughout the patch.


> +    int width = 0, height = 0, par_n = 0, par_d = 0;
> +    double dpar_n = 0, dpar_d = 0;
> +    GstCaps *caps = gst_buffer_get_caps (m_buffer);

I would like to see these much needed enhancements to the MediaPlayer also
contribute to getting it more in line with the coding standards. So I would
prefer to have these variable names matching those we expect elsewhere
(http://webkit.org/coding/coding-style.html). So, par_n/par_d become
pixelAspectRatioNumerator/pixelAspectRatioDenominator, and so on. Would be good
to declare each in its own line after this change.

> +    if (!gst_video_format_parse_caps (GST_BUFFER_CAPS (m_buffer), NULL,
&width, &height) ||
> +	   !gst_video_parse_caps_pixel_aspect_ratio(caps, &par_n, &par_d))
> +	 return;

More spaces.

> -    cairo_set_source_surface(cr, m_surface, 0, 0);
> +    cairo_scale (cr, dpar_n / dpar_d, dpar_d / dpar_n);

^

>	   friend gboolean mediaPlayerPrivateErrorCallback(GstBus* bus,
GstMessage* message, gpointer data);
>	   friend gboolean mediaPlayerPrivateEOSCallback(GstBus* bus,
GstMessage* message, gpointer data);
>	   friend gboolean mediaPlayerPrivateStateCallback(GstBus* bus,
GstMessage* message, gpointer data);
> +	friend void mediaPlayerPrivateRepaintCallback(WebKitVideoSink*,
GstBuffer *buffer, MediaPlayerPrivate* playerPrivate);
>  

Indentation is off here.
> +    if (G_UNLIKELY (!GST_BUFFER_CAPS (buffer))) {
> +	 buffer = gst_buffer_make_metadata_writable (buffer);
> +	 gst_buffer_set_caps (buffer, GST_PAD_CAPS (GST_BASE_SINK_PAD (sink)));

>      }

Indentation.

> -    GST_CALL_PARENT_WITH_DEFAULT(GST_BASE_SINK_CLASS, unlock,
> +    return GST_CALL_PARENT_WITH_DEFAULT(GST_BASE_SINK_CLASS, unlock,
>				    (object), TRUE);

Indentation of the second line should be updated here.

>  static void
> +marshal_VOID__MINIOBJECT (GClosure * closure, GValue * return_value,
> +    guint n_param_values, const GValue * param_values, gpointer
invocation_hint,
> +    gpointer marshal_data)
> +{

Identation is weird, and there's a space.

> +  typedef void (*marshalfunc_VOID__MINIOBJECT) (gpointer obj, gpointer arg1,

> +	 gpointer data2);

Indentation is also off in this function, I think you can just leave data2 in
the same line, webkit doesn't strive to be under 80 cols =).

r- for now for the reasons above, thanks for working on this! =)