Bug 77087

Summary: [GStreamer] 0.11 video-sink
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebKitGTKAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: David.Ronis, eric.carlson, feature-media-reviews, gns, levin+threading, menard, mrobinson, ossy, pnormand, slomo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 77089, 79651    
Bug Blocks: 77005    
Attachments:
Description Flags
0.11 video-sink
mrobinson: review-
0.11 video-sink
pnormand: commit-queue-
0.11 video-sink
none
0.11 video-sink
none
0.11 video-sink
pnormand: commit-queue-
0.11 video-sink
none
Patch
gyuyoung.kim: commit-queue-
Patch
webkit-ews: commit-queue-
Patch
none
Patch
none
video-sink
none
video-sink
none
video-sink
mrobinson: review-
Patch
webkit-ews: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch mrobinson: review+, mrobinson: commit-queue-

Description Philippe Normand 2012-01-26 02:58:04 PST
SSIA
Comment 1 Philippe Normand 2012-01-26 03:25:10 PST
Created attachment 124093 [details]
0.11 video-sink
Comment 2 WebKit Review Bot 2012-01-26 03:27:35 PST
Attachment 124093 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:67:  webkit_video_sink_signals is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:71:  timeout_id is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:72:  buffer_mutex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:73:  data_cond is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:331:  need_pool is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:362:  n_param_values is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:363:  invocation_hint is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:387:  gobject_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:388:  gstbase_sink_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:389:  element_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 10 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Sebastian Dröge (slomo) 2012-01-30 02:40:49 PST
Comment on attachment 124093 [details]
0.11 video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=124093&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:30
> +    return adoptRef(new ImageGStreamer(buffer, size));

Why don't you use the format at all here?

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:51
>          kCGBitmapByteOrder32Little | kCGImageAlphaFirst, provider.get(), 0, false, kCGRenderingIntentDefault);

I think it makes sense to use the format here

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:36
>          cairoFormat = CAIRO_FORMAT_RGB24;

This is not true, GST_ARGB is CAIRO_ARGB32 on big endian systems and GST_BGRA is CAIRO_ARGB32 on little endian systems. Same for the one without alpha channel.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:55
>      m_image = BitmapImage::create(surface);

Is it necessary to keep the buffer around until the object is destroyed? Same question for the other implementations

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:35
>          imageFormat = QImage::Format_RGB32;

Same about endianness here too probably

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:100
> +#if GLIB_CHECK_VERSION(2, 31, 0)

We depend on that version in GStreamer 0.11 anyway

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:182
> +        gst_buffer_copy_into(newBuffer, buffer, (GstBufferCopyFlags) GST_BUFFER_COPY_ALL, 0, bufferSize);

You copy twice here. Just create and allocate a new buffer and fill the new buffer in the loop below

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:351
> +        gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_META);

Do you handle the video meta in the request-repaint signal handlers? There might be any possible stride used if you add this meta instead of the old GST_ROUND_UP_4(width) strides.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:356
> +    gst_query_add_allocation_meta(query, GST_VIDEO_META_API);

You might want to add support for the crop meta too here
Comment 4 Martin Robinson 2012-01-30 09:30:41 PST
Comment on attachment 124093 [details]
0.11 video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=124093&action=review

I've listed a bunch of style nits below. In general, GObject method names should use camelCase except for the few that cannot be (webkit_video_sink_class_init and webkit_video_sink_init). Be sure to use full words for variable names everywhere and function headers must be one line.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:40
> +    unsigned char* bufferData = reinterpret_cast<unsigned char*>(info.data);

Shouldn't you use Uint8* here as well?

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:47
> +    unsigned char* bufferData = reinterpret_cast<unsigned char*>(info.data);

Should you use uchar* here as well?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1530
> +    format = GST_VIDEO_INFO_FORMAT(&info);
> +    width = GST_VIDEO_INFO_WIDTH(&info);
> +    height = GST_VIDEO_INFO_HEIGHT(&info);

I'd move the delcaration of width, height and format to these lines.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1533
> +    if (!gst_video_format_parse_caps(caps, &format, &width, &height))

And here as well.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:32
> +#include "config.h"
> +
> +#include "VideoSinkGStreamer.h"

No extra line necessary here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:35
> +#if USE(GSTREAMER)
> +#ifdef GST_API_VERSION_1

Please combine these into a compound conditional.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:49
> +#if G_BYTE_ORDER == G_LITTLE_ENDIAN
> +#define VIDEO_FORMATS "{ BGRx, BGRA }"
> +#else
> +#define VIDEO_FORMATS "{ xRGB, ARGB }"
> +#endif

Please use a staticconst char* for this.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:51
> +                                                                   GST_PAD_SINK, GST_PAD_ALWAYS,

This can be one line.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:56
> +GST_DEBUG_CATEGORY_STATIC(webkit_video_sink_debug);
> +#define GST_CAT_DEFAULT webkit_video_sink_debug

I'm not sure I understand this.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:67
>> +static guint webkit_video_sink_signals[LAST_SIGNAL] = { 0, };
> 
> webkit_video_sink_signals is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

Please fix the style bot not to complain about this. We do similar things for WebKit2. Take a look.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:84
> +    gboolean unlocked;

Please use bool here and true/false when assigning.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:95
> +static void
> +webkit_video_sink_init(WebKitVideoSink* sink)

This should be one line.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:113
> +static gboolean
> +webkit_video_sink_timeout_func(gpointer data)

Ditto.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:124
> +    if (!buffer || priv->unlocked || G_UNLIKELY(!GST_IS_BUFFER(buffer))) {

You should use the WTF version of unlikely here, if possible.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:139
> +static GstFlowReturn
> +webkit_video_sink_render(GstBaseSink* bsink, GstBuffer* buffer)

One line also. :)

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:151
> +    priv->buffer = gst_buffer_ref(buffer);

Could you use a RefPtr for this?

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:172
> +        // could be passed multiple times to this method (in theory)

Missing a period here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:176
> +        // Check if allocation failed

Missing a period here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:194
> +        const guint8* source = (const guint8*) sourceInfo.data;
> +        gst_buffer_map(newBuffer, &destinationInfo, GST_MAP_WRITE);
> +        guint8* destination = (guint8*) destinationInfo.data;

Please use a static_cast here or const_cast if necessary.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:229
> +                                          (GDestroyNotify)gst_object_unref);

You should use reinterpret_cast.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:248
> +#if GLIB_CHECK_VERSION(2, 31, 0)
> +        g_cond_clear(priv->data_cond);
> +        WTF::fastDelete(priv->data_cond);
> +#else
> +        g_cond_free(priv->data_cond);
> +#endif

I'd like to see these abstracted into a helper method.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:266
> +static void
> +unlock_buffer_mutex(WebKitVideoSinkPrivate* priv)

You should use camelCase for static methods.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:286
> +    WebKitVideoSink* sink = WEBKIT_VIDEO_SINK(object);
> +
> +    unlock_buffer_mutex(sink->priv);

This can be one line.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:289
> +    return GST_CALL_PARENT_WITH_DEFAULT(GST_BASE_SINK_CLASS, unlock,
> +                                        (object), TRUE);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:296
> +    WebKitVideoSink* sink = WEBKIT_VIDEO_SINK(object);
> +    WebKitVideoSinkPrivate* priv = sink->priv;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:303
> +    return GST_CALL_PARENT_WITH_DEFAULT(GST_BASE_SINK_CLASS, unlock_stop,
> +                                        (object), TRUE);

Ditto.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:361
> +marshal_VOID__MINIOBJECT(GClosure * closure, GValue * return_value,

Please give this and the enclosed variables WebKit style names and ensure the asterisk placement is correct.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:411
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),

use static_cast here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:415
> +            0,
> +            0,
> +            0,
> +            marshal_VOID__MINIOBJECT,

For parameters like these I think it's useful to put what each one does in a comment.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:425
> +/**
> + * webkit_video_sink_new:
> + *
> + * Creates a new GStreamer video sink.
> + *
> + * Return value: a #GstElement for the newly created video sink
> + */

I don't think we need gtkdoc since this isn't public API.
Comment 5 Philippe Normand 2012-02-06 09:18:48 PST
Comment on attachment 124093 [details]
0.11 video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=124093&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:55
>>      m_image = BitmapImage::create(surface);
> 
> Is it necessary to keep the buffer around until the object is destroyed? Same question for the other implementations

I don't think so because the ImageGStreamer ctors copy the buffer data to platform-dependant representations of the Image.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:182
>> +        gst_buffer_copy_into(newBuffer, buffer, (GstBufferCopyFlags) GST_BUFFER_COPY_ALL, 0, bufferSize);
> 
> You copy twice here. Just create and allocate a new buffer and fill the new buffer in the loop below

Oh hum I meant to copy only the metadata here, so I'll use  GST_BUFFER_COPY_METADATA instead of _ALL :)

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:248
>> +#endif
> 
> I'd like to see these abstracted into a helper method.

Well I'm not sure it's really needed. As Sebastian points out GStreamer 0.11 requires a quite recent glib anyway and these g_cond_ calls are not used in lots of places.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:351
>> +        gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_META);
> 
> Do you handle the video meta in the request-repaint signal handlers? There might be any possible stride used if you add this meta instead of the old GST_ROUND_UP_4(width) strides.

Hum I don't use it in the signal handler. I guess it's best to remove this option from the pool then?

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:356
>> +    gst_query_add_allocation_meta(query, GST_VIDEO_META_API);
> 
> You might want to add support for the crop meta too here

Is it really needed?
Comment 6 Sebastian Dröge (slomo) 2012-02-07 04:03:09 PST
(In reply to comment #5)

> >> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:351
> >> +        gst_buffer_pool_config_add_option(config, GST_BUFFER_POOL_OPTION_VIDEO_META);
> > 
> > Do you handle the video meta in the request-repaint signal handlers? There might be any possible stride used if you add this meta instead of the old GST_ROUND_UP_4(width) strides.
> 
> Hum I don't use it in the signal handler. I guess it's best to remove this option from the pool then?

Or support and use the metadata in the signal handler. Which would be preferred because it will give you better performance. And as you use cairo, supporting random strides should be quite easy.

> >> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:356
> >> +    gst_query_add_allocation_meta(query, GST_VIDEO_META_API);
> > 
> > You might want to add support for the crop meta too here
> 
> Is it really needed?

No, but if you support it, it will give you much better performance if cropping is required somewhere (e.g. non-multiple of 16 widths/heights in h264 or Theora). I remember that adding cropping support to xvimagesink and gst-ffmpeg for this gave ~25% performance improvement for some videos. And cropping support is really easy if you use cairo for rendering anyway.
Comment 7 Philippe Normand 2012-02-27 02:07:17 PST
Created attachment 128989 [details]
0.11 video-sink
Comment 8 WebKit Review Bot 2012-02-27 02:10:01 PST
Attachment 128989 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:86:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:97:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:217:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:250:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:258:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:268:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:273:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:282:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:322:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:357:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:39:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 13 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Philippe Normand 2012-02-27 02:25:54 PST
Comment on attachment 128989 [details]
0.11 video-sink

Attachment 128989 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11628917
Comment 10 Philippe Normand 2012-02-27 02:26:54 PST
Created attachment 128993 [details]
0.11 video-sink
Comment 11 Philippe Normand 2012-02-27 02:28:56 PST
Created attachment 128995 [details]
0.11 video-sink

Should make style-bot happier.
Comment 12 Philippe Normand 2012-02-27 02:35:33 PST
Created attachment 128997 [details]
0.11 video-sink

Added forward declarations in ImageGStreamer.h and moved includes to implementations.
Comment 13 Philippe Normand 2012-02-27 02:40:57 PST
Comment on attachment 128997 [details]
0.11 video-sink

Attachment 128997 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11630956
Comment 14 Alexis Menard (darktears) 2012-02-27 02:42:20 PST
(In reply to comment #13)
> (From update of attachment 128997 [details])
> Attachment 128997 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11630956

Did you try on the Qt port or should I?
Comment 15 Philippe Normand 2012-02-27 02:50:59 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 128997 [details] [details])
> > Attachment 128997 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/11630956
> 
> Did you try on the Qt port or should I?

Not yet, I probably should, and will :)
You can try too but you first need to build gst-0.11 git branches.
Comment 16 Philippe Normand 2012-02-27 02:58:15 PST
Created attachment 129001 [details]
0.11 video-sink

Fix build.
Comment 17 Philippe Normand 2012-02-27 03:04:09 PST
Comment on attachment 129001 [details]
0.11 video-sink

Attachment 129001 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11628945
Comment 18 Early Warning System Bot 2012-02-27 03:16:40 PST
Comment on attachment 129001 [details]
0.11 video-sink

Attachment 129001 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11627987
Comment 19 Gyuyoung Kim 2012-02-27 03:57:27 PST
Comment on attachment 129001 [details]
0.11 video-sink

Attachment 129001 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11628967
Comment 20 Philippe Normand 2012-02-27 08:23:39 PST
Those build errors are expected because patch of bug 77089 is not landed yet.
Comment 21 Philippe Normand 2012-02-27 08:26:22 PST
Created attachment 129041 [details]
Patch
Comment 22 Gyuyoung Kim 2012-02-27 08:30:48 PST
Comment on attachment 129041 [details]
Patch

Attachment 129041 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11645025
Comment 23 Philippe Normand 2012-02-27 10:13:04 PST
Created attachment 129063 [details]
Patch
Comment 24 Early Warning System Bot 2012-02-27 10:18:45 PST
Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11639137
Comment 25 Gustavo Noronha (kov) 2012-02-27 10:32:08 PST
Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11644084
Comment 26 Gyuyoung Kim 2012-02-27 10:47:12 PST
Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11641100
Comment 27 Philippe Normand 2012-02-27 11:26:16 PST
Created attachment 129075 [details]
Patch
Comment 28 Philippe Normand 2012-03-02 03:30:08 PST
Created attachment 129865 [details]
Patch
Comment 29 Philippe Normand 2012-03-06 09:41:15 PST
Created attachment 130394 [details]
video-sink
Comment 30 Sebastian Dröge (slomo) 2012-03-15 02:19:05 PDT
Comment on attachment 130394 [details]
video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=130394&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:80
> +        bitmapInfo = kCGBitmapByteOrder32Big | kCGImageAlphaFirst;

Just curious but can't you support ABGR and ARGB and RGBA and RGB (and all the other variants) here by setting the correct bitmapInfo value here? And maybe even the xRGB variants?

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:76
> +    int stride = cairo_format_stride_for_width(cairoFormat, width);

This is not necessarily correct. The GStreamer stride for the two formats you support here is always GST_ROUND_UP_4 (width * 3) or GST_ROUND_UP_4 (width * 4)

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:363
> +            marshal_VOID__MINIOBJECT, // Custom marshaller.

Why don't you use the generic GLib marshaller here? g_cclosure_marshal_generic() or how it's called
Comment 31 Philippe Normand 2012-03-15 03:11:31 PDT
Comment on attachment 130394 [details]
video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=130394&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:80
>> +        bitmapInfo = kCGBitmapByteOrder32Big | kCGImageAlphaFirst;
> 
> Just curious but can't you support ABGR and ARGB and RGBA and RGB (and all the other variants) here by setting the correct bitmapInfo value here? And maybe even the xRGB variants?

Hum not sure, I'll give this code another shot.

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:76
>> +    int stride = cairo_format_stride_for_width(cairoFormat, width);
> 
> This is not necessarily correct. The GStreamer stride for the two formats you support here is always GST_ROUND_UP_4 (width * 3) or GST_ROUND_UP_4 (width * 4)

Ok I'll just use GST_ROUND_UP and handle both cases then :)

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:363
>> +            marshal_VOID__MINIOBJECT, // Custom marshaller.
> 
> Why don't you use the generic GLib marshaller here? g_cclosure_marshal_generic() or how it's called

I've just copied this code from the other video-sink. I think you wrote this actually ;)
But yeah I guess the signal definition can be simplified indeed.
Comment 32 Philippe Normand 2012-03-15 03:12:19 PDT
Comment on attachment 130394 [details]
video-sink

Needs work after Sebastian's review.
Comment 33 Philippe Normand 2012-04-16 03:35:00 PDT
I'm having issues testing the ImageGstreamerCG changes. Since this code is not enabled in default builds (and I doubt anyone actually uses it these days) I'll remove it and update it in a follow-up patch. It will remain in the SVN history anyway, in case anyone cares :)
Comment 34 Philippe Normand 2012-04-16 06:18:15 PDT
Created attachment 137331 [details]
video-sink
Comment 35 WebKit Review Bot 2012-04-16 06:21:02 PDT
Attachment 137331 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 36 Philippe Normand 2012-04-16 06:51:15 PDT
Created attachment 137337 [details]
video-sink

Fix code style.
Comment 37 Martin Robinson 2012-04-19 16:43:25 PDT
Comment on attachment 137337 [details]
video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=137337&action=review

r- mostly for the code duplication. Below is list of quite picky style nits. :)

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:63
> +        ImageGStreamer(GstBuffer*&, GstCaps*);

Probably should just use a pointer here.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:66
> +    if (format == GST_VIDEO_FORMAT_BGRA)
> +        cairoFormat = CAIRO_FORMAT_ARGB32;
> +    else
> +        cairoFormat = CAIRO_FORMAT_RGB24;

It might be better to express this as a one-liner:

cairoFormat = (format == GST_VIDEO_FORMAT_BGRA) ? CAIRO_FORMAT_ARGB32 : CAIRO_FORMAT_RGB24;

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:71
> +    if (format == GST_VIDEO_FORMAT_ARGB)
>          cairoFormat = CAIRO_FORMAT_ARGB32;
>      else
>          cairoFormat = CAIRO_FORMAT_RGB24;

Ditto.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:81
> +    int stride;
> +    if (format == GST_VIDEO_FORMAT_BGRA)
> +        stride = GST_ROUND_UP_4(width * 4);
> +    else
> +        stride = GST_ROUND_UP_4(width * 3);

This might be cleaner as a single line:

int stride = GST_ROUND_UP_4(width * (format == GST_VIDEO_FORMAT_BGRA ? 4 : 3));

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:89
> +

Extra newline here.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:50
> +    GstVideoInfo videoInfo;
> +    gst_video_info_from_caps(&videoInfo, caps);
>  
> -    gst_caps_unref(caps);
> +    GstVideoFormat format = GST_VIDEO_INFO_FORMAT(&videoInfo);
> +    int width = GST_VIDEO_INFO_WIDTH(&videoInfo);
> +    int height = GST_VIDEO_INFO_HEIGHT(&videoInfo);
>  
> +    GstMapInfo info;
> +    gst_buffer_map(buffer, &info, GST_MAP_READ);
> +    uchar* bufferData = reinterpret_cast<uchar*>(info.data);

This seems common to both implementations. Could you move it to a place that's platform independent and share it?

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:56
> +    uchar* bufferData = reinterpret_cast<uchar*>(GST_BUFFER_DATA(buffer));
> +    int width = 0, height = 0;
> +    GstVideoFormat format;
> +    gst_video_format_parse_caps(caps, &format, &width, &height);
> +#endif

Ditto.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:62
> +    if (format == GST_VIDEO_FORMAT_BGRA)
> +        imageFormat = QImage::Format_RGB32;
> +    else
>          imageFormat = QImage::Format_RGB888;

You could also play the same trick with one liners here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:30
>  G_BEGIN_DECLS

You shouldn't need G_BEGIN_DECLS/G_END_DECLS in this file.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:78
>  GType       webkit_video_sink_get_type(void) G_GNUC_CONST;

No need for the void here. It makes sense for this declaration to use WebKit style.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:84
> +#ifndef GST_API_VERSION_1
>  GstElement *webkit_video_sink_new(WebCore::GStreamerGWorld*);
> +#else
> +GstElement *webkit_video_sink_new();
> +#endif
>  

Your asterisk is in the wrong place here. I think in this case it would be cleaner to have both constructors take the GStreamerGWorld argument (if it exists for both). In the new constructor, just could just leave it unused.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:67
> +    GstBuffer* buffer;
> +    guint timeout_id;
> +    GMutex* buffer_mutex;
> +    GCond* data_cond;
> +    GstVideoInfo info;
> +

You should use WebKit style for these variable names.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:88
> +    WebKitVideoSinkPrivate* priv;

No need to declare your variable here, just declare it when you use it.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:102
> +    WebKitVideoSinkPrivate* priv = sink->priv;
> +    GstBuffer* buffer;

Ditto with these variables.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:123
> +static GstFlowReturn webkit_video_sink_render(GstBaseSink* bsink, GstBuffer* buffer)

bsink -> baseSink?

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:156
> +    // Cairo's ARGB has pre-multiplied alpha while GStreamer's doesn't.
> +    // Here we convert to Cairo's ARGB.
> +    if (format == GST_VIDEO_FORMAT_ARGB || format == GST_VIDEO_FORMAT_BGRA) {
> +        // Because GstBaseSink::render() only owns the buffer reference in the
> +        // method scope we can't use gst_buffer_make_writable() here. Also
> +        // The buffer content should not be changed here because the same buffer
> +        // could be passed multiple times to this method (in theory).

Instead of duplicating this code between both implementations it should be shared somehow. perhaps a ultity file or a VideoSinkGstreamerShared.cpp.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:159
> +        gsize bufferSize = gst_buffer_get_size(buffer);
> +        GstBuffer* newBuffer = gst_buffer_new_and_alloc(bufferSize);
> +

The code here looks like it should be moved to a helper function.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:183
> +                alpha = source[3];

You should define alpha here instead of above.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:214
> +    // This should likely use a lower priority, but glib currently starves
> +    // lower priority sources.
> +    // See: https://bugzilla.gnome.org/show_bug.cgi?id=610830.
> +    priv->timeout_id = g_timeout_add_full(G_PRIORITY_DEFAULT, 0,
> +                                          webkit_video_sink_timeout_func,
> +                                          gst_object_ref(sink),
> +                                          reinterpret_cast<GDestroyNotify>(gst_object_unref));
> +

This can also be shared.
Comment 38 Philippe Normand 2012-05-01 10:51:39 PDT
Comment on attachment 137337 [details]
video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=137337&action=review

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:50
>> +    uchar* bufferData = reinterpret_cast<uchar*>(info.data);
> 
> This seems common to both implementations. Could you move it to a place that's platform independent and share it?

What do you want me to refactor/share exactly?

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:56
>> +#endif
> 
> Ditto.

Hum, not sure what you mean, a new utility function in GStreamerVersioning to parse video caps?

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:102
>> +    GstBuffer* buffer;
> 
> Ditto with these variables.

Ok for the buffer variable, but the 2 others are used immediately after. So... :)

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:156
>> +        // could be passed multiple times to this method (in theory).
> 
> Instead of duplicating this code between both implementations it should be shared somehow. perhaps a ultity file or a VideoSinkGstreamerShared.cpp.

I'll investigate this, maybe GStreamerVersioning can be used too.
Comment 39 Martin Robinson 2012-05-01 11:55:40 PDT
Comment on attachment 137337 [details]
video-sink

View in context: https://bugs.webkit.org/attachment.cgi?id=137337&action=review

Sorry for the bad review habits. I've tried to clarify it a bit below.

>>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:50
>>> +    uchar* bufferData = reinterpret_cast<uchar*>(info.data);
>> 
>> This seems common to both implementations. Could you move it to a place that's platform independent and share it?
> 
> What do you want me to refactor/share exactly?

For instance you could create a helper like

static void ImageGStreamer::getVideoSizeAndFormatFromCaps(GstCaps*, IntRect& size, GstVideoFormat& format);

This helper could be used by both the Qt and Cairo versions of this code (instead of the copypasta) and know all about the different GStreamer versions. This would keep those details out of all the platform specific files and reduce the size of the patch.

>>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer1.cpp:102
>>> +    GstBuffer* buffer;
>> 
>> Ditto with these variables.
> 
> Ok for the buffer variable, but the 2 others are used immediately after. So... :)

Fair enough. :)
Comment 40 Philippe Normand 2012-05-02 00:35:26 PDT
(In reply to comment #39)
> (From update of attachment 137337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137337&action=review
> 
> Sorry for the bad review habits. I've tried to clarify it a bit below.
> 
> >>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:50
> >>> +    uchar* bufferData = reinterpret_cast<uchar*>(info.data);
> >> 
> >> This seems common to both implementations. Could you move it to a place that's platform independent and share it?
> > 
> > What do you want me to refactor/share exactly?
> 
> For instance you could create a helper like
> 
> static void ImageGStreamer::getVideoSizeAndFormatFromCaps(GstCaps*, IntRect& size, GstVideoFormat& format);
> 
> This helper could be used by both the Qt and Cairo versions of this code (instead of the copypasta) and know all about the different GStreamer versions. This would keep those details out of all the platform specific files and reduce the size of the patch.
> 

Ok, yes it makes a lot of sense :) Actually I might implement that function in GStreamerVersioning and use it in both the media player and ImageGStreamer.
Comment 41 Philippe Normand 2012-05-03 05:55:06 PDT
(In reply to comment #30)
> (From update of attachment 130394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130394&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:80
> > +        bitmapInfo = kCGBitmapByteOrder32Big | kCGImageAlphaFirst;
> 
> Just curious but can't you support ABGR and ARGB and RGBA and RGB (and all the other variants) here by setting the correct bitmapInfo value here? And maybe even the xRGB variants?
> 
> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:76
> > +    int stride = cairo_format_stride_for_width(cairoFormat, width);
> 
> This is not necessarily correct. The GStreamer stride for the two formats you support here is always GST_ROUND_UP_4 (width * 3) or GST_ROUND_UP_4 (width * 4)
> 

The GStreamer stride might like this yes but on Cairo side bothCAIRO_FORMAT_ARGB32 and  CAIRO_FORMAT_RGB24 assume each pixel is a 32-bit quantity but for the RGB24 format the upper 8 bits are unused. So this means, I think, that the stride used to create the Cairo surface should always be GST_ROUND_UP_4(width * 4) in our case.
Comment 42 Sebastian Dröge (slomo) 2012-05-03 06:03:42 PDT
(In reply to comment #41)
> (In reply to comment #30)
> > (From update of attachment 130394 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130394&action=review
> > 
> > > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCG.mm:80
> > > +        bitmapInfo = kCGBitmapByteOrder32Big | kCGImageAlphaFirst;
> > 
> > Just curious but can't you support ABGR and ARGB and RGBA and RGB (and all the other variants) here by setting the correct bitmapInfo value here? And maybe even the xRGB variants?
> > 
> > > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:76
> > > +    int stride = cairo_format_stride_for_width(cairoFormat, width);
> > 
> > This is not necessarily correct. The GStreamer stride for the two formats you support here is always GST_ROUND_UP_4 (width * 3) or GST_ROUND_UP_4 (width * 4)
> > 
> 
> The GStreamer stride might like this yes but on Cairo side bothCAIRO_FORMAT_ARGB32 and  CAIRO_FORMAT_RGB24 assume each pixel is a 32-bit quantity but for the RGB24 format the upper 8 bits are unused. So this means, I think, that the stride used to create the Cairo surface should always be GST_ROUND_UP_4(width * 4) in our case.

Ah so you only support the cairo formats. In that case you want GST_VIDEO_FORMAT_xRGB and GST_VIDEO_FORMAT_ARGB on big endian architectures and GST_VIDEO_FORMAT_BGRx and GST_VIDEO_FORMAT_BGRA on little endian architectures.

The default stride for them on the GStreamer side will be width*4 (obviously not necessary to round up to a multiple of four as it already will be a multiple of four ;) ). If there's GstVideoMeta attached to the buffer the stride might be different (larger) though but cairo can happily support this.

I hope this helps?
Comment 43 Philippe Normand 2012-05-03 06:33:09 PDT
Yes, it's all clear now!
I'll upload a new version of this patch adressing the latest reviews.
Comment 44 Philippe Normand 2012-05-03 07:07:38 PDT
Created attachment 140004 [details]
Patch
Comment 45 WebKit Review Bot 2012-05-03 07:10:18 PDT
Attachment 140004 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:75:  webkit_video_sink_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:78:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:80:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 46 Early Warning System Bot 2012-05-03 07:13:31 PDT
Comment on attachment 140004 [details]
Patch

Attachment 140004 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12606177
Comment 47 Early Warning System Bot 2012-05-03 07:14:37 PDT
Comment on attachment 140004 [details]
Patch

Attachment 140004 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12608241
Comment 48 Philippe Normand 2012-05-03 07:32:01 PDT
Created attachment 140009 [details]
Patch

Qt build fix.
Comment 49 WebKit Review Bot 2012-05-03 07:34:43 PDT
Attachment 140009 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:75:  webkit_video_sink_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:78:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:80:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Philippe Normand 2012-05-13 19:19:55 PDT
Created attachment 141630 [details]
Patch
Comment 51 WebKit Review Bot 2012-05-13 19:22:52 PDT
Attachment 141630 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:75:  webkit_video_sink_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:78:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.h:80:  webkit_video_sink_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Martin Robinson 2012-05-16 11:19:09 PDT
Comment on attachment 141630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141630&action=review

Okay. This looks good to me, though it's hard to review it thoroughly since it's such a big patch. It looks like most of the work is mechanical. The biggest change I'd like to see (other than a lot of nit picky stylistic stuff) is to isolate as much of the version dependent #ifndef/#else stuff into helper functions. This will make it easier to understand long methods, because they won't be laced with these preprocessor commands.

> Source/WebCore/ChangeLog:10
> +        Video frame rendering changes for GStreamer 0.11/1.0. The
> +        ImageGStreamerCG abstraction is being removed until I manage to
> +        port my gst-mac WebKit branch over to WebKit2 mac port.

Do you mind splitting the CG removal into another patch? r+ in advance.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:57
> +            return FloatRect(0, 0, -1, -1);

A comment here explaining these values might be good. Are these numbers that Gstreamer expects?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1505
> +            const gchar * const * extensions;

The asterisks should be attached to the typenames here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:187
> +#ifndef GST_API_VERSION_1
>      // For the unlikely case where the buffer has no caps, the caps
>      // are implicitely the caps of the pad. This shouldn't happen.
> -    if (G_UNLIKELY(!GST_BUFFER_CAPS(buffer))) {
> +    if (UNLIKELY(!GST_BUFFER_CAPS(buffer))) {
>          buffer = priv->buffer = gst_buffer_make_metadata_writable(priv->buffer);
> -        gst_buffer_set_caps(priv->buffer, GST_PAD_CAPS(GST_BASE_SINK_PAD(bsink)));
> +        gst_buffer_set_caps(priv->buffer, GST_PAD_CAPS(GST_BASE_SINK_PAD(baseSink)));
>      }
>  
>      GstCaps *caps = GST_BUFFER_CAPS(buffer);
>      GstVideoFormat format;
>      int width, height;
> -    if (G_UNLIKELY(!gst_video_format_parse_caps(caps, &format, &width, &height))) {
> +    if (UNLIKELY(!gst_video_format_parse_caps(caps, &format, &width, &height))) {
> +        gst_buffer_unref(buffer);
> +        g_mutex_unlock(priv->bufferMutex);
> +        return GST_FLOW_ERROR;
> +    }
> +#else
> +    GstVideoFrame frame;
> +    if (!gst_video_frame_map(&frame, &priv->info, buffer, GST_MAP_READ)) {

I think it makes sense to split this off into a helper function like, getFormatAndDimensions().

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:214
> +#ifndef GST_API_VERSION_1
>          GstBuffer *newBuffer = gst_buffer_try_new_and_alloc(GST_BUFFER_SIZE(buffer));
> +#else
> +        gsize bufferSize = gst_buffer_get_size(buffer);
> +        GstBuffer* newBuffer = gst_buffer_new_and_alloc(bufferSize);
> +#endif
>  

Again this would probably be better as a helper called, createBuffer.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:225
> +#ifndef GST_API_VERSION_1
> +        gst_buffer_copy_metadata(newBuffer, buffer, static_cast<GstBufferCopyFlags>(GST_BUFFER_COPY_ALL));
> +#else
> +        gst_buffer_copy_into(newBuffer, buffer, static_cast<GstBufferCopyFlags>(GST_BUFFER_COPY_METADATA), 0, bufferSize);
> +#endif

It might even make sense to isolate all  #ifdeffed sections in this method into helpers.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:233
>          const guint8 *source = GST_BUFFER_DATA(buffer);
>          guint8 *destination = GST_BUFFER_DATA(newBuffer);

The asterisks are in the wrong place here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:266
> +#ifdef GST_API_VERSION_1
> +        gst_buffer_unmap(buffer, &sourceInfo);
> +        gst_buffer_unmap(newBuffer, &destinationInfo);
> +#endif

Could you eliminate these calls by creating an OwnPtr for GstBuffer?

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:271
> +

Extra line?

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:365
> +static gboolean webkit_video_sink_propose_allocation(GstBaseSink* baseSink, GstQuery* query)

New method should have WebKit styled naming conventions, so this can be webkitVideoSinkProposeAllocation.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:369
> +    gboolean need_pool;

need_pool -> needPool.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:387
>  static void
>  marshal_VOID__MINIOBJECT(GClosure * closure, GValue * return_value,

You might consider fixing the style here too.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:447
> +            0, // Class offset.
> +            0, // Accumulator.
> +            0, // Accumulator data.
> +#ifndef GST_API_VERSION_1

Super nit: No need for periods here since these aren't sentences.
Comment 53 Philippe Normand 2012-05-23 10:49:39 PDT
Comment on attachment 141630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141630&action=review

>> Source/WebCore/ChangeLog:10
>> +        port my gst-mac WebKit branch over to WebKit2 mac port.
> 
> Do you mind splitting the CG removal into another patch? r+ in advance.

Sure!

>> Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:57
>> +            return FloatRect(0, 0, -1, -1);
> 
> A comment here explaining these values might be good. Are these numbers that Gstreamer expects?

These numbers are what the GraphicsContext draw method expect in the case where there is no crop rectangle.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1505
>> +            const gchar * const * extensions;
> 
> The asterisks should be attached to the typenames here.

Right. Anyway, this code is no longer necessary since the mimeTypeCache() refactoring.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:187
>> +    if (!gst_video_frame_map(&frame, &priv->info, buffer, GST_MAP_READ)) {
> 
> I think it makes sense to split this off into a helper function like, getFormatAndDimensions().

Well there is already, webkitGetVideoSizeAndFormatFromCaps but I couldn' t use it with a GstVideoFrame.
I fully understand the motivation for these utility functions but IMHO they restrain what the GStreamer APIs allow us to do. Will we need a utility function for each way to get those information using gst APIs?

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:214
>>  
> 
> Again this would probably be better as a helper called, createBuffer.

Sure.

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:233
>>          guint8 *destination = GST_BUFFER_DATA(newBuffer);
> 
> The asterisks are in the wrong place here.

Indeed!

>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:266
>> +#endif
> 
> Could you eliminate these calls by creating an OwnPtr for GstBuffer?

Hum.... I need to think about that and how to integrate this with GstMapInfo.
Comment 54 Martin Robinson 2012-05-23 15:02:16 PDT
Comment on attachment 141630 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141630&action=review

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:32
> +bool webkitGetVideoSizeAndFormatFromCaps(GstCaps*, WebCore::IntSize&, GstVideoFormat&, int&, int&, int&);

There's no need to prefix this helper with "webkit." Generic types like int& should include the variable name so it's possible to see how to call the helper by looking at the header. The style checker only complains in situations like "GstVideoFormat& format" where there is some repetition.

>>> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:187
>>> +    if (!gst_video_frame_map(&frame, &priv->info, buffer, GST_MAP_READ)) {
>> 
>> I think it makes sense to split this off into a helper function like, getFormatAndDimensions().
> 
> Well there is already, webkitGetVideoSizeAndFormatFromCaps but I couldn' t use it with a GstVideoFrame.
> I fully understand the motivation for these utility functions but IMHO they restrain what the GStreamer APIs allow us to do. Will we need a utility function for each way to get those information using gst APIs?

Hrm. In this case perhaps it makes sense to create helper called getVideoSizeAndFormatFromBuffer(buffer, int) and then for GST < 0.11 have it call getVidoeSizeAndFormatFromCaps.
Comment 55 Philippe Normand 2012-05-26 15:16:54 PDT
I'll send the code style WebKit-ification as a separate patch too and focus only on the 0.11 port for this one.
Comment 56 Philippe Normand 2012-06-08 12:50:02 PDT
*** Bug 88578 has been marked as a duplicate of this bug. ***
Comment 57 Philippe Normand 2012-06-09 11:52:21 PDT
(In reply to comment #52)
> 
> > Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:266
> > +#ifdef GST_API_VERSION_1
> > +        gst_buffer_unmap(buffer, &sourceInfo);
> > +        gst_buffer_unmap(newBuffer, &destinationInfo);
> > +#endif
> 
> Could you eliminate these calls by creating an OwnPtr for GstBuffer?
> 

Hum, how would I do that taking into account the GstMapInfo? I'm not sure how to integrate this notion in an OwnPtr.

For now I'll send the revised patch minus this update.
Comment 58 Philippe Normand 2012-06-09 11:54:50 PDT
Created attachment 146714 [details]
Patch
Comment 59 WebKit Review Bot 2012-06-09 11:57:04 PDT
Attachment 146714 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/gstreamer..." exit_code: 1
Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:84:  gst_buffer_try__and_alloc is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 Philippe Normand 2012-06-09 11:58:11 PDT
Created attachment 146715 [details]
Patch
Comment 61 Simon Hausmann 2012-06-10 02:14:26 PDT
Comment on attachment 146715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146715&action=review

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:39
> +    QPixmap* surface = new QPixmap;

This seems like an unused and leaked variable :)
Comment 62 Philippe Normand 2012-06-10 10:23:50 PDT
(In reply to comment #61)
> (From update of attachment 146715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146715&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerQt.cpp:39
> > +    QPixmap* surface = new QPixmap;
> 
> This seems like an unused and leaked variable :)

It's used later on to create the BitmapImage. About leakage, maybe it'd be convenient to use an OwnPtr?
Comment 63 Philippe Normand 2012-06-18 17:44:07 PDT
Comment on attachment 146715 [details]
Patch

Pulling out of review queue. I need to rework some parts of the patch.
Comment 64 Philippe Normand 2012-06-18 22:05:45 PDT
Created attachment 148244 [details]
Patch
Comment 65 Philippe Normand 2012-06-18 22:07:28 PDT
New patch, rebased against trunk, with ChangeLogs (hrm) and a small change in webkitwebsrc (gst_tag_list_free -> _unref).
Comment 66 Martin Robinson 2012-06-18 22:34:06 PDT
Comment on attachment 148244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148244&action=review

This version is a lot better! I'm still a bit sad to see all of the #ifdefs splitting everything though. :( Please fix the following things before landing.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:52
> +bool webkitGetVideoSizeAndFormatFromCaps(GstCaps* caps, WebCore::IntSize& size, GstVideoFormat& format, int& pixelAspectRatioNumerator, int& pixelAspectRatioDenominator, int& stride)

Please don't use the webkit prefix here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:80
> +GstBuffer* webkitGstCreateBuffer(GstBuffer* buffer)

This should probably be called createGstBuffer.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:90
> +    if (!newBuffer)
> +        return 0;

ASSERT(newBuffer) maybe. Do you handle returning zero at the callers? If not, this should be an assert instead of a return.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:33
> +typedef struct _GstBuffer GstBuffer;
> +typedef struct _GstCaps GstCaps;

GstVersioning.h now includes gst.h, so perhaps you can get rid of these typedefs now?

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:33
>  #include <wtf/gobject/GOwnPtr.h>

This should go before the conditional includes according to WebKit style, I believe.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:39
> +    : m_image(0)

m_image is a RefPtr so you don't need to initialize it here.

> Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:68
> +    GstVideoCropMeta* cropMeta = gst_buffer_get_video_crop_meta(buffer);
> +    if (cropMeta)
> +        setCropRect(FloatRect(cropMeta->x, cropMeta->y, cropMeta->width, cropMeta->height));

This can simply be:
if (GstVideoCropMeta* cropMeta = gst_buffer_get_video_crop_meta(buffer))
    setCropRect(FloatRect(cropMeta->x, cropMeta->y, cropMeta->width, cropMeta->height));

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:502
> +    int pixelAspectRatioNumerator, pixelAspectRatioDenominator, stride;

It'd be better to move these down to where you first use them.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:505
>      int displayWidth, displayHeight, displayAspectRatioGCD;
> -    int originalWidth = 0, originalHeight = 0;
> +    IntSize originalSize;
> +    GstVideoFormat format;

The same for all of these variables really.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:349
> +    WebKitVideoSink* sink = WEBKIT_VIDEO_SINK(baseSink);

Might as well move sink down to where it's first used.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:403
> +#ifdef GST_API_VERSION_1
> +    gst_element_class_set_metadata(elementClass,
> +#else
>      gst_element_class_set_details_simple(elementClass,
> +#endif
>              "WebKit video sink",
>              "Sink/Video", "Sends video data from a GStreamer pipeline to a Cairo surface",
>              "Alp Toker <alp@atoker.com>");

It'd be better to wrap this into a helper method like:

void setGstElementClassMetadata(GstElementClass* elementClass, const char* name, const char* longName, const char* description, const char* author)
{
#ifdef GST_API_VERSION_1
    gst_element_class_set_metadata(elementClass, name, longNmae, description, author);
#else
    gst_element_class_set_details_simple(elementClass, name, longNmae, description, author);
#endif
}

It's a bit icky here to have the #ifdef breaking the actual line of code. I think the goal should be to move all of this code to more and more isolated helpers that abstract away the differences between the versions.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:428
> +            g_cclosure_marshal_generic, // Custom marshaller

I think you can probably remove the comment here.

> configure.ac:351
> -GSTREAMER_1_0_REQUIRED_VERSION=1.0
> +GSTREAMER_1_0_REQUIRED_VERSION=0.11.90

Why is this fix necessary?
Comment 67 Philippe Normand 2012-06-19 19:15:43 PDT
(In reply to comment #66)
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:90
> > +    if (!newBuffer)
> > +        return 0;
> 
> ASSERT(newBuffer) maybe. Do you handle returning zero at the callers? If not, this should be an assert instead of a return.
> 

The zero test is handled by the caller yes.

> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamer.h:33
> > +typedef struct _GstBuffer GstBuffer;
> > +typedef struct _GstCaps GstCaps;
> 
> GstVersioning.h now includes gst.h, so perhaps you can get rid of these typedefs now?
> 

Oh yes, good point.

> > Source/WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:33
> >  #include <wtf/gobject/GOwnPtr.h>
> 
> This should go before the conditional includes according to WebKit style, I believe.
> 

Indeed!
 
> 
> > configure.ac:351
> > -GSTREAMER_1_0_REQUIRED_VERSION=1.0
> > +GSTREAMER_1_0_REQUIRED_VERSION=0.11.90
> 
> Why is this fix necessary?

Because the current Version value in gstreamer-1.0.pc is not 1.0 yet.
Thanks for the review!
Comment 68 Philippe Normand 2012-06-19 19:26:20 PDT
Committed r120790: <http://trac.webkit.org/changeset/120790>
Comment 69 Csaba Osztrogonác 2012-06-19 22:22:52 PDT
(In reply to comment #68)
> Committed r120790: <http://trac.webkit.org/changeset/120790>

And Qt buildfix landed in http://trac.webkit.org/changeset/120799
Comment 70 Philippe Normand 2012-06-19 22:44:52 PDT
(In reply to comment #69)
> (In reply to comment #68)
> > Committed r120790: <http://trac.webkit.org/changeset/120790>
> 
> And Qt buildfix landed in http://trac.webkit.org/changeset/120799

Oops. Thanks Ossy!