[Webkit-unassigned] [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 19 14:07:01 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=115352
Andre Moreira Magalhaes <andrunko at gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #208831|0 |1
is obsolete| |
--- Comment #29 from Andre Moreira Magalhaes <andrunko at gmail.com> 2013-08-19 14:06:30 PST ---
Created an attachment (id=209121)
--> (https://bugs.webkit.org/attachment.cgi?id=209121&action=review)
Patch
(In reply to comment #28)
> (From update of attachment 208831 [details])
> 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.
>
Fixed.
> > 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.
>
Fixed.
> > 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.
>
Done, made StreamingClient a base class now.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:73
> > +// common StreamingClient interface
>
> Comments should start with capital letter and finish with a period.
>
Fixed, removed this one.
> > 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.
>
Done.
> > 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.
>
Done, using from base class now.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:99
> > + StreamingClientHandler* m_handler;
>
> If we use the handler this should be OwnPtr<StreamingClientHandler>
>
No handler anymore.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:124
> > +
> > + WebKitWebSrc* m_src;
> > + StreamingClientHandler* m_handler;
>
> Ditto.
>
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?
>
This is not needed. The thing here is that the element will go to NULL state before dispose, thus stop will be called and all resources freed.
If we kept this code and the element dispose were called before setting the state to NULL many other members (everything reset on stop) would leak, not only the buffer.
> > 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.
>
Added a new class GMutexLocker (generic) in WTF/wtf/gobject/ as discussed with Gustavo Noronha and changed the gst webkitsrc code to use it instead.
> > 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.
>
Done.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:443
> > + gboolean seeking;
>
> Declare this when first used, and use bool instead of gboolean.
>
Done.
> > 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.
>
Done.
> > 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.
>
Done.
> > 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.
>
Done, also changed other places that were using g_timeout_add to use g_idle_add.
> > 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.
>
Done.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:926
> > +// StreamingClientHandler
>
> This comment looks redundant to me.
>
Removed.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
> > + gst_object_unref(m_src);
> > + m_src = 0;
>
> We should use GRefPtr for this
>
Done.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1124
> > +// CachedResourceStreamingClient
>
> This comment looks redundant too.
>
Removed.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1127
> > + , m_handler(new StreamingClientHandler(src))
>
> This should use OwnPtr and adopt the pointer here.
>
Not needed as we dont have a handler anymore.
> > 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.
>
Done.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1151
> > + return (!m_resource);
>
> I don't think the parentheses are need here.
>
Removed.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1191
> > +// ResourceHandleStreamingClient
>
> This comment looks also redundant.
>
Removed.
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:1194
> > + , m_handler(new StreamingClientHandler(src))
>
> Same comment about OwnPtr
Also not needed anymore.
Thanks for the review, new patch attached.
I also ran some manual tests and tested the regressions on bug #117141 and bug #117688 and everything looks fine.
$ Tools/Scripts/run-webkit-tests --no-new-test-results --debug-rwt-logging --no-show-results --no-sample-on-timeout --results-directory layout-test-results --debug --webkit-test-runner --gtk fast/js/custom-constructors.html fast/js/cyclic-proto.html --iteration=100
...
17:39:00.966 22661 Testing completed, Exit status: 0
=> Results: 200/200 tests passed (100.0%)
=> Tests to be fixed (0):
=> Tests that will only be fixed if they crash (WONTFIX) (0):
--
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