[webkit-reviews] review denied: [Bug 115352] [gstreamer] Make sure gstreamer source element is thread-safe : [Attachment 209131] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Andre Moreira Magalhaes
<andrunko at gmail.com>'s request for review:
Bug 115352: [gstreamer] Make sure gstreamer source element is thread-safe
https://bugs.webkit.org/show_bug.cgi?id=115352

Attachment 209131: Patch
https://bugs.webkit.org/attachment.cgi?id=209131&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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?