[Webkit-unassigned] [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=115352





--- Comment #35 from Andre Moreira Magalhaes <andrunko at gmail.com>  2013-08-22 07:40:27 PST ---
(In reply to comment #34)
> (From update of attachment 209131 [details])
> 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);
> 
Tbh, I didn't want to create this class, I just did it cause Carlos requested on comment #28.

We could make the "lock" method private but we need a public "unlock" method at least as there are some cases we need to unlock it manually.

What do you suggest here? Should I make "lock" private? Should I revert this change and use GST_OBJECT_LOCK as it was before?

Note that having only a public "unlock" method would mean that once the GMutexLocker is unlocked it cannot be reused anymore, e.g.:

  {
    GMutexLocker foo(bar);
    doSomethingThatRequiresLock();
    foo.unlock();

    doSomethingElse();

    // cannot lock bar again using "foo" locker, need to create a new one.
    GMutexLocker foo1(bar);
    doSomethingThatRequiresLock();
  }

> > 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 elementissues 
> > + *   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?
I will work on a fix for the other comments in the meantime.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.