[Webkit-unassigned] [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 20 15:11:19 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=115352
Benjamin Poulain <benjamin at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #209131|review? |review-
Flag| |
--- Comment #34 from Benjamin Poulain <benjamin at webkit.org> 2013-08-20 15:10:47 PST ---
(From update of attachment 209131)
View in context: https://bugs.webkit.org/attachment.cgi?id=209131&action=review
> Source/WTF/wtf/gobject/GMutexLocker.h:39
> + inline GMutexLocker()
> + : m_mutex(0)
> + {
> + }
This is unused.
> Source/WTF/wtf/gobject/GMutexLocker.h:42
> + : m_mutex(0)
m_mutex(mutex)
> Source/WTF/wtf/gobject/GMutexLocker.h:62
> + inline void lock(GMutex *mutex)
> + {
> + m_mutex = mutex;
> + if (m_mutex)
> + g_mutex_lock(m_mutex);
> + }
> +
> + inline void unlock()
> + {
> + if (m_mutex) {
> + g_mutex_unlock(m_mutex);
> + m_mutex = 0;
> + }
> + }
Looks like this should be private.
You have nothing preventing bad use with the current design:
GMutexLocker foo;
foo.lock(bar);
foo.lock(baz);
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:56
> +/* Premisses:
> + * - webkitsrc may be created from any thread inside gstreamer
> + * - webkitsrc may be created without a player involved (e.g.: dash gstreamer element
> + * internally downloading stream fragments)
> + * - streaming client holds reference to src, so that src is never deleted while client exists
> + * - if the src exists, appsrc also exists
> + * - streaming client is created on start
> + * - streaming client is deleted on stop and cancels resource
> + * - streaming client callbacks are always invoked from main thread
> + */
Wrong comment style.
Honestly this comment does not help. It should have explanation on why your premisses matter.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:60
> +// StreamingClient base class.
Useless comment.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:86
> + bool loadFailed() const;
> + void setDefersLoading(bool);
Missing Overrides.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:93
> + char* getOrCreateReadBuffer(CachedResource*, size_t requestedSize, size_t& actualSize);
> + void responseReceived(CachedResource*, const ResourceResponse&);
> + void dataReceived(CachedResource*, const char*, int);
> + void notifyFinished(CachedResource*);
ditto.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:106
> + bool loadFailed() const;
> + void setDefersLoading(bool);
ditto.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:117
> + char* getOrCreateReadBuffer(size_t requestedSize, size_t& actualSize);
> + void willSendRequest(ResourceHandle*, ResourceRequest&, const ResourceResponse&);
> + void didReceiveResponse(ResourceHandle*, const ResourceResponse&);
> + void didReceiveData(ResourceHandle*, const char*, int, int);
> + void didFinishLoading(ResourceHandle*, double /*finishTime*/);
> + void didFail(ResourceHandle*, const ResourceError&);
> + void wasBlocked(ResourceHandle*);
> + void cannotShowURL(ResourceHandle*);
ditto.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:492
> +// Must be called on main thread and with object unlocked.
You should have assertions instead of a comment.
> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:549
> + if (priv->player) {
> + if (CachedResourceLoader* loader = priv->player->cachedResourceLoader())
> + priv->client = new CachedResourceStreamingClient(src, loader, request);
This looks odd. If you have priv->player but no loader, you don't create the client?
Is that intended?
--
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