[Webkit-unassigned] [Bug 29997] [GStreamer] Prevent double-memcpy in GStreamer media player

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 12 05:27:07 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=29997


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41024|review?                     |review-
               Flag|                            |




--- Comment #3 from Gustavo Noronha (kov) <gns at gnome.org>  2009-10-12 05:27:07 PDT ---
(From update of attachment 41024)
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! =)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


More information about the webkit-unassigned mailing list