[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