[Webkit-unassigned] [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 16 01:31:33 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=115352
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #208831| |review-
Flag| |
--- Comment #28 from Carlos Garcia Campos <cgarcia at igalia.com> 2013-08-16 01:31:05 PST ---
(From update of attachment 208831)
View in context: https://bugs.webkit.org/attachment.cgi?id=208831&action=review
> Source/WebCore/ChangeLog:7
> + Patch by Andre Moreira Magalhaes <andre.magalhaes at collabora.co.uk> on 2013-08-14
> + Reviewed by Philippe Normand.
Please, don't submit this as reviewed, since it contains new code that hasn't been reviewed.
> Source/WebCore/ChangeLog:37
> + * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> + (_WebKitWebSrcPrivate):
> + (webkit_web_src_init):
> + (webKitWebSrcFinalize):
> + (webKitWebSrcSetProperty):
> + (webKitWebSrcGetProperty):
> + (webKitWebSrcStop):
> + (webKitWebSrcStart):
> + (webKitWebSrcChangeState):
> + (webKitWebSrcQueryWithParent):
> + (webKitWebSrcGetUri):
> + (webKitWebSrcSetUri):
> + (webKitWebSrcNeedDataMainCb):
> + (webKitWebSrcEnoughDataMainCb):
> + (webKitWebSrcSeekMainCb):
> + (webKitWebSrcSeekDataCb):
> + (webKitWebSrcSetMediaPlayer):
> + (StreamingClient::StreamingClient):
> + (StreamingClient::~StreamingClient):
> + (StreamingClient::didReceiveResponse):
> + (StreamingClient::didReceiveData):
> + (StreamingClient::didFinishLoading):
> + (StreamingClient::wasBlocked):
> + (StreamingClient::cannotShowURL):
This doesn't look up to date. You should explain briefly here what you are changing.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:71
> +class StreamingClientHandler {
> + public:
> + StreamingClientHandler(WebKitWebSrc*);
> + virtual ~StreamingClientHandler();
> +
> + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize);
> + void responseReceived(const ResourceResponse&);
> + void dataReceived(const char*, int);
> + void notifyFinished();
> +
> + private:
> + WebKitWebSrc* m_src;
> +};
I wonder if we could add the common implementation to the base class instead of using a new class.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:73
> +// common StreamingClient interface
Comments should start with capital letter and finish with a period.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:80
> + virtual bool loadFailed() const = 0;
> + virtual void setDefersLoading(bool) = 0;
> +};
So, instead of an abstract class, we could probably add the common implementation here.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:98
> WebKitWebSrc* m_src;
This is already in the handler, maybe we could add a getter to the handler, or if we end up using a base class, move this to the base class as protected member.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:99
> + StreamingClientHandler* m_handler;
If we use the handler this should be OwnPtr<StreamingClientHandler>
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:124
> +
> + WebKitWebSrc* m_src;
> + StreamingClientHandler* m_handler;
Ditto.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-288
> - if (priv->buffer) {
> -#ifdef GST_API_VERSION_1
> - unmapGstBuffer(priv->buffer.get());
> -#endif
> - priv->buffer.clear();
> - }
Why don't we need this anymore?
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:371
> + GST_OBJECT_LOCK(src);
> priv->iradioMode = g_value_get_boolean(value);
> + GST_OBJECT_UNLOCK(src);
I wonder if we could add a simple GstMutexLocker class, to simply the locks, you wouldn't need to unlock the object on every function return, for example.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:439
> +// must be called on main thread and with object unlocked
Comments should start with capital letter and finish with a period.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:443
> + gboolean seeking;
Declare this when first used, and use bool instead of gboolean.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:500
> +// must be called on main thread and with object unlocked
Comments should start with capital letter and finish with a period.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:558
> + CachedResourceLoader* loader = priv->player->cachedResourceLoader();
> + if (loader)
> + priv->client = new CachedResourceStreamingClient(src, loader, request);
if (CachedResourceLoader* loader = priv->player->cachedResourceLoader())
priv->client = new CachedResourceStreamingClient(src, loader, request);
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:607
> + priv->startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStart, gst_object_ref(src), (GDestroyNotify) gst_object_unref);
You can use g_idle_add_full with G_PRIORITY_DEFAULT instead of a timeout source with 0 delay. You should use C++ style casts for GSourceFunc and GDestroyNotify.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:615
> + priv->stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitWebSrcStop, gst_object_ref(src), (GDestroyNotify) gst_object_unref);
Ditto.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:638
> + if ((format == GST_FORMAT_BYTES) && (src->priv->size > 0)) {
I know this was already like this, but now that you are modifying it we can take advantage to remove the extra parentheses.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926
> +// StreamingClientHandler
This comment looks redundant to me.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
> + gst_object_unref(m_src);
> + m_src = 0;
We should use GRefPtr for this
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1124
> +// CachedResourceStreamingClient
This comment looks redundant too.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1127
> + , m_handler(new StreamingClientHandler(src))
This should use OwnPtr and adopt the pointer here.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1138
> + gst_object_unref(m_src);
> + m_src = 0;
This should use GRefPtr. I think this should be done in the parent class or the handler, but not in both places.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1151
> + return (!m_resource);
I don't think the parentheses are need here.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1191
> +// ResourceHandleStreamingClient
This comment looks also redundant.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1194
> + , m_handler(new StreamingClientHandler(src))
Same comment about OwnPtr
--
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