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

[bugzilla-daemon at webkit.org]

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 137337: video-sink
https://bugs.webkit.org/attachment.cgi?id=137337&action=review

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