[webkit-reviews] review denied: [Bug 77087] [GStreamer] 0.11 video-sink : [Attachment 124093] 0.11 video-sink

Mon Jan 30 09:30:40 PST 2012

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 77087: [GStreamer] 0.11 video-sink
https://bugs.webkit.org/show_bug.cgi?id=77087

Attachment 124093: 0.11 video-sink
https://bugs.webkit.org/attachment.cgi?id=124093&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.