[webkit-reviews] review denied: [Bug 99065] [GStreamer] Add support for Media Source API : [Attachment 216400] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Stephane Jadaud
<sjadaud at sii.fr>'s request for review:
Bug 99065: [GStreamer] Add support for Media Source API
https://bugs.webkit.org/show_bug.cgi?id=99065

Attachment 216400: Patch
https://bugs.webkit.org/attachment.cgi?id=216400&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216400&action=review


Can this feature be limited to gst 1.x? We plan to remove support for 0.10 so
I'd rather not increase the amount of to-be-removed code.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:92
> +MediaPlayer::ReadyState MediaSourceGStreamer::readyState() const
> +{
> +    return m_readyState;
> +}
> +
> +void MediaSourceGStreamer::setReadyState(MediaPlayer::ReadyState readyState)

> +{
> +    m_readyState = readyState;
> +}

Simple getters and setters like this should be implemented in the header file.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:17
> + *	  * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Are you sure this copyright is what you want to use?

>
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:15
> + *	  * Neither the name of Google Inc. nor the names of its

Ditto

>
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:75
> +MediaPlayer::ReadyState SourceBufferPrivateGStreamer::readyState() const
> +{
> +    return m_readyState;
> +}
> +
> +void SourceBufferPrivateGStreamer::setReadyState(MediaPlayer::ReadyState
readyState)
> +{
> +    m_readyState = readyState;
> +}

Ditto about implementation in header

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:36
> + * - webkitsrc may be created from any thread inside gstreamer

This is false, un-needed copy/paste from webkitsrc

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:52
> +typedef struct _Sources {

Why plural?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:55
> +    guint sourceid;	      /* To control the GSource */

sourceId

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:68
> +    guint startID;
> +    guint stopID;
> +    guint needDataID;
> +    guint enoughDataID;
> +    guint seekID;

Can these be renamed as well?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:78
> +    Sources source[MAX_SOURCES];

sources

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:164

> +    GstPad* gpad;

ghostPad

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:471

> +	   priv->source[MEDIA_TYPE_VIDEO].startID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaVideoSrcStart, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);
> +	   priv->source[MEDIA_TYPE_AUDIO].startID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaAudioSrcStart, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

It'd be good to give a name to each source

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:478

> +	   priv->source[MEDIA_TYPE_VIDEO].stopID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaVideoSrcStop, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);
> +	   priv->source[MEDIA_TYPE_AUDIO].stopID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaAudioSrcStop, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:693

> +    priv->source[MEDIA_TYPE_VIDEO].needDataID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaVideoSrcNeedDataMainCb, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:710

> +    priv->source[MEDIA_TYPE_AUDIO].needDataID =
g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc)
webKitMediaAudioSrcNeedDataMainCb, gst_object_ref(src), (GDestroyNotify)
gst_object_unref);

And here too

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:888

> +    priv->duration = duration >= 0.0 ? (gint64)(duration*GST_SECOND) : 0;

Please use C++ style casts

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:897

> +    if (!strncmp(type, "video", 5)) {

Please use a String here

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:911

> +	   priv->source[MEDIA_TYPE_VIDEO].buffer =
adoptGRef(createGstBufferForData(data, length));
> +	   GST_OBJECT_UNLOCK(m_src);
> +
> +	   ret =
gst_app_src_push_buffer(GST_APP_SRC(priv->source[MEDIA_TYPE_VIDEO].appsrc),
> +	       priv->source[MEDIA_TYPE_VIDEO].buffer.leakRef());

What's the point of keeping the buffer in the source if it's release just after
being pushed?