[Webkit-unassigned] [Bug 99065] [GStreamer] Add support for Media Source API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Nov 12 03:27:03 PST 2013
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.
More information about the webkit-unassigned
mailing list