Changeset 230627 in webkit

Timestamp:

Apr 13, 2018 3:09:23 AM (6 years ago)

Author:

magomez@igalia.com

Message:

[GTK] [gstreamer] video won't unpause when built with -DUSE_GSTREAMER_GL=OFF
​https://bugs.webkit.org/show_bug.cgi?id=183362

Reviewed by Carlos Garcia Campos.

Remove the drawCancelled flag and use a new one to indicate that the player is being destroyed.
That new flag is only enabled on destruction and it's not modified by cancelRepaint(), which
can be used to handle the pause event without avoiding future renderings. Also cancelRepaint()
has only effect when not in AC mode.

Covered by existent tests.

• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:

(WebCore::MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase):
(WebCore::MediaPlayerPrivateGStreamerBase::triggerRepaint):
(WebCore::MediaPlayerPrivateGStreamerBase::cancelRepaint):

• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp (modified) (6 diffs)
• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-04-13  Miguel Gomez  <magomez@igalia.com>
+
+        [GTK] [gstreamer] video won't unpause when built with -DUSE_GSTREAMER_GL=OFF
+        https://bugs.webkit.org/show_bug.cgi?id=183362
+
+        Reviewed by Carlos Garcia Campos.
+
+        Remove the drawCancelled flag and use a new one to indicate that the player is being destroyed.
+        That new flag is only enabled on destruction and it's not modified by cancelRepaint(), which
+        can be used to handle the pause event without avoiding future renderings. Also cancelRepaint()
+        has only effect when not in AC mode.
+
+        Covered by existent tests.
+
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
+        (WebCore::MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase):
+        (WebCore::MediaPlayerPrivateGStreamerBase::triggerRepaint):
+        (WebCore::MediaPlayerPrivateGStreamerBase::cancelRepaint):
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:
+
 2018-04-13  Xabier Rodriguez Calvar  <calvaris@igalia.com>
 

trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp

@@ -241 +241 @@
 MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase()
 {
+    // Flag the player as being destroyed, so triggerRepaint will ignore incoming samples.
+    m_destroying = true;
+
 #if ENABLE(ENCRYPTED_MEDIA)
     m_protectionCondition.notifyAll();
@@ -259 +262 @@
         g_signal_handlers_disconnect_matched(m_volumeElement.get(), G_SIGNAL_MATCH_DATA, 0, 0, nullptr, nullptr, this);
 
-    // This will release the GStreamer thread from m_drawCondition if AC is disabled.
+    // This will release the GStreamer thread from m_drawCondition in non AC mode in case there's an ongoing triggerRepaint call.
     cancelRepaint();
 
@@ -785 +788 @@
 void MediaPlayerPrivateGStreamerBase::triggerRepaint(GstSample* sample)
 {
+    // Do not try to process new frames if the player is being destroyed by the main thread.
+    if (m_destroying)
+        return;
+
     bool triggerResize;
     {
@@ -799 +806 @@
     if (!m_renderingCanBeAccelerated) {
         LockHolder locker(m_drawMutex);
-        if (m_drawCancelled)
-            return;
         m_drawTimer.startOneShot(0_s);
         m_drawCondition.wait(m_drawMutex);
@@ -812 +817 @@
     {
         LockHolder lock(m_drawMutex);
-        if (m_drawCancelled)
-            return;
         if (!m_platformLayerProxy->scheduleUpdateOnCompositorThread([this] { this->pushTextureToCompositor(); }))
             return;
@@ -829 +832 @@
 void MediaPlayerPrivateGStreamerBase::cancelRepaint()
 {
-    LockHolder locker(m_drawMutex);
-
+    // The goal of this function is to release the GStreamer thread from m_drawCondition in triggerRepaint() in non-AC case,
+    // to avoid a deadlock if the player gets paused while waiting for drawing (see https://bugs.webkit.org/show_bug.cgi?id=170003):
+    // the main thread is waiting for the GStreamer thread to pause, but the GStreamer thread is locked waiting for the
+    // main thread to draw. This deadlock doesn't happen when using AC because the sample is processed (not painted) in the compositor
+    // thread, so the main thread can request the pause and wait if the GStreamer thread is waiting for the compositor thread.
     if (!m_renderingCanBeAccelerated) {
         m_drawTimer.stop();
-    }
-
-    m_drawCancelled = true;
-    m_drawCondition.notifyOne();
+        LockHolder locker(m_drawMutex);
+        m_drawCondition.notifyOne();
+    }
 }
 

trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h

@@ -236 +236 @@
     Condition m_drawCondition;
     Lock m_drawMutex;
-    bool m_drawCancelled { false };
+    bool m_destroying { false };
     RunLoop::Timer<MediaPlayerPrivateGStreamerBase> m_drawTimer;