[GStreamer] 0.11 video-sink

[Philippe Normand]

SSIA

[Philippe Normand]

Created attachment 124093 [details]
0.11 video-sink

[WebKit Review Bot]

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.

[Sebastian Dröge (slomo)]

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

[Martin Robinson]

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.

[Philippe Normand]

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?

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

Created attachment 128989 [details]
0.11 video-sink

[WebKit Review Bot]

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.

[Philippe Normand]

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

[Philippe Normand]

Created attachment 128993 [details]
0.11 video-sink

[Philippe Normand]

Created attachment 128995 [details]
0.11 video-sink

Should make style-bot happier.

[Philippe Normand]

Created attachment 128997 [details]
0.11 video-sink

Added forward declarations in ImageGStreamer.h and moved includes to implementations.

[Philippe Normand]

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

[Alexis Menard (darktears)]

(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?

[Philippe Normand]

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

[Philippe Normand]

Created attachment 129001 [details]
0.11 video-sink

Fix build.

[Philippe Normand]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Philippe Normand]

Those build errors are expected because patch of bug 77089 is not landed yet.

[Philippe Normand]

Created attachment 129041 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 129041 [details]
Patch

Attachment 129041 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11645025

[Philippe Normand]

Created attachment 129063 [details]
Patch

[Early Warning System Bot]

Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11639137

[Gustavo Noronha (kov)]

Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11644084

[Gyuyoung Kim]

Comment on attachment 129063 [details]
Patch

Attachment 129063 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11641100

[Philippe Normand]

Created attachment 129075 [details]
Patch

[Philippe Normand]

Created attachment 129865 [details]
Patch

[Philippe Normand]

Created attachment 130394 [details]
video-sink

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

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.

[Philippe Normand]

Comment on attachment 130394 [details]
video-sink

Needs work after Sebastian's review.

[Philippe Normand]

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 :)

[Philippe Normand]

Created attachment 137331 [details]
video-sink

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 137337 [details]
video-sink

Fix code style.

[Martin Robinson]

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.

[Philippe Normand]

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.

[Martin Robinson]

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

[Philippe Normand]

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

[Philippe Normand]

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

[Sebastian Dröge (slomo)]

(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?

[Philippe Normand]

Yes, it's all clear now!
I'll upload a new version of this patch adressing the latest reviews.

[Philippe Normand]

Created attachment 140004 [details]
Patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 140004 [details]
Patch

Attachment 140004 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12606177

[Early Warning System Bot]

Comment on attachment 140004 [details]
Patch

Attachment 140004 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12608241

[Philippe Normand]

Created attachment 140009 [details]
Patch

Qt build fix.

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 141630 [details]
Patch

[WebKit Review Bot]

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.

[Martin Robinson]

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.

[Philippe Normand]

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.

[Martin Robinson]

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.

[Philippe Normand]

I'll send the code style WebKit-ification as a separate patch too and focus only on the 0.11 port for this one.

[Philippe Normand]

*** Bug 88578 has been marked as a duplicate of this bug. ***

[Philippe Normand]

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

[Philippe Normand]

Created attachment 146714 [details]
Patch

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 146715 [details]
Patch

[Simon Hausmann]

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 :)

[Philippe Normand]

(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?

[Philippe Normand]

Comment on attachment 146715 [details]
Patch

Pulling out of review queue. I need to rework some parts of the patch.

[Philippe Normand]

Created attachment 148244 [details]
Patch

[Philippe Normand]

New patch, rebased against trunk, with ChangeLogs (hrm) and a small change in webkitwebsrc (gst_tag_list_free -> _unref).

[Martin Robinson]

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 <[e-mail]>");

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?

[Philippe Normand]

(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!

[Philippe Normand]

Committed r120790: <http://trac.webkit.org/changeset/120790>

[Csaba Osztrogonác]

(In reply to comment #68)
> Committed r120790: <http://trac.webkit.org/changeset/120790>

And Qt buildfix landed in http://trac.webkit.org/changeset/120799

[Philippe Normand]

(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!