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

• Component: Media
• Status: RESOLVED FIXED

[Sebastian Dröge (slomo)]

Hi,
currently the GStreamer media player memcpy's the video data twice but this isn't really needed.

Currently the video sink has an internal surface to which all buffers are painted and then in MediaPlayerPrivate::paint() this surface is painted to some external cairo_t. Instead of doing that I would propose:

in MediaPlayerPrivate::paint() get the buffers from the sink's async queue, check lateness and all that, scale (for par, etc) and then paint it to the cairo_t. This might need some exposing of information from the video sink to the media player.

[Sebastian Dröge (slomo)]

Created attachment 41021 [details]
double-memcpy.diff

[Sebastian Dröge (slomo)]

Created attachment 41024 [details]
double-memcpy.diff

[Gustavo Noronha (kov)]

Comment on attachment 41024 [details]
double-memcpy.diff

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! =)

[Sebastian Dröge (slomo)]

(In reply to comment #3)
> (From update of attachment 41024 [details])
> 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?

I just copied this from the video sink. This patch shouldn't change anything in functionality.

So, the surface *to* which we're painting is still ARGB32. Only the GStreamer video sink only accepts non-alpha RGB, which means that you can still do nice alpha effects with the video but not from inside the GStreamer pipeline. (Note: almost no video format supports ARGB)

But I plan to add ARGB32 support to the video sink after you accepted my two patches. This will be quite inefficient because GStreamer and Cairo have other conventions for ARGB but for the rare case where you have alpha-content in the GStreamer pipeline it will preserve the alpha.

(Too much information?)

> > -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.

Ok... PARNumerator would be fine too? I mean, everybody who should touch this code should know what PAR means... and pixelAspectRatio is a long word ;)

[Gustavo Noronha (kov)]

(In reply to comment #4)

> So, the surface *to* which we're painting is still ARGB32. Only the GStreamer
> video sink only accepts non-alpha RGB, which means that you can still do nice
> alpha effects with the video but not from inside the GStreamer pipeline. (Note:
> almost no video format supports ARGB)
> 
> But I plan to add ARGB32 support to the video sink after you accepted my two
> patches. This will be quite inefficient because GStreamer and Cairo have other
> conventions for ARGB but for the rare case where you have alpha-content in the
> GStreamer pipeline it will preserve the alpha.
> 
> (Too much information?)

I guess I was able to get it, thanks for explaining!

> Ok... PARNumerator would be fine too? I mean, everybody who should touch this
> code should know what PAR means... and pixelAspectRatio is a long word ;)

yeah, I know, but we have much longer variable names in WebKit =D. I think PARNumerator is fine, though, given that PAR is a concept everyone touching the code should grasp, as you pointed.

[Sebastian Dröge (slomo)]

Created attachment 41037 [details]
double-memcpy.diff

[Sebastian Dröge (slomo)]

Created attachment 41039 [details]
double-memcpy.diff

[Gustavo Noronha (kov)]

Comment on attachment 41039 [details]
double-memcpy.diff

Thanks!

[WebKit Commit Bot]

Comment on attachment 41039 [details]
double-memcpy.diff

Clearing flags on attachment: 41039

Committed r49438: <http://trac.webkit.org/changeset/49438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.