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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Sebastian Dröge <slomo at circular-chaos.org>  2009-10-12 06:04:21 PDT ---
(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 ;)

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