[webkit-reviews] review granted: [Bug 197558] REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos : [Attachment 369484] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 197558: REGRESSION(r243197): [GStreamer] Web process hangs when scrolling
twitter timeline which contains HLS videos
https://bugs.webkit.org/show_bug.cgi?id=197558

Attachment 369484: Patch

https://bugs.webkit.org/attachment.cgi?id=369484&action=review




--- Comment #6 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 369484
  --> https://bugs.webkit.org/attachment.cgi?id=369484
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369484&action=review

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:73
> +		   GST_CAT_LEVEL_LOG(webkit_media_player_debug,
GST_LEVEL_WARNING, m_object.get(), "Notifier was invalidated, bailing out");

Do you think this notification should never happen? I don't think it's so weird
that a task has been enqueued, notifier becomes invalid and the task is run.
Wouldn't it be interesting to make this at least INFO?

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:97
> +	      
m_synchronousNotificationCondition.wait(m_synchronousNotificationMutex);

Considering we are waking up everybody below, shouldn't we check for the
notification flag to be still active? Maybe a conditional wait?

> Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:158
> +    GRefPtr<GstObject> m_object;

I think this should be called m_gstObject.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:614
> +    GST_DEBUG_OBJECT(src, "Closing session, loader: %p, resource: %p",
priv->loader.get(), priv->resource.get());

You could put this as else of the next if

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:649
> +	   return TRUE;

Shouldn't we unref the event before this here?