[Webkit-unassigned] [Bug 99065] [GStreamer] Add support for Media Source API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99065


Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #216400|review?                     |review-
               Flag|                            |




--- Comment #54 from Philippe Normand <pnormand at igalia.com>  2013-11-12 03:25:42 PST ---
(From update of attachment 216400)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.