Changeset 249321 in webkit

Timestamp:

Aug 30, 2019 4:02:18 AM (5 years ago)

Author:

cturner@igalia.com

Message:

[GStreamer] Do not ref the player count from background threads.
​https://bugs.webkit.org/show_bug.cgi?id=201222

Reviewed by Xabier Rodriguez-Calvar.

Test: imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-playback-retrieve-persistent-license.https.html

In the sync-message handler, a ref() was being taken waiting for a
CDM instance to be attached. This hits asserts since you are not
allowed to ref() an object created on the main thread
(BasePlayer) on a background thread.

The protection condition was overly scoped, tidied up the locking
and made it more granular. To avoid needing to hold a ref() in the
background thread, use instead a semaphore to signal when a CDM
instance is attached, or the player has been destroyed.

Also remove an erroneous safe-guard, the operator= in
isCDMInstanceAvailable will ref() the CDMInstance for us. This use
of holding a reference to CDMInstance in the decryptors is not
thread-safe, and now we have a problem since there's no clean way
to communicate with CDMInstance from background threads without
being thread unsafe. For ClearKey and Widevine, a thread safe
ProxyCDM needs to be designed and passed to background
threads (upcoming patch).

• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:

(WebCore::MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase):
(WebCore::MediaPlayerPrivateGStreamerBase::handleSyncMessage):
(WebCore::MediaPlayerPrivateGStreamerBase::cdmInstanceAttached):
(WebCore::MediaPlayerPrivateGStreamerBase::cdmInstanceDetached):
(WebCore::MediaPlayerPrivateGStreamerBase::handleProtectionEvent):

• platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:
• platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:

(isCDMInstanceAvailable):

Location:

trunk/Source/WebCore

Files:

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

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-08-30  Charlie Turner  <cturner@igalia.com>
+
+        [GStreamer] Do not ref the player count from background threads.
+        https://bugs.webkit.org/show_bug.cgi?id=201222
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Test: imported/w3c/web-platform-tests/encrypted-media/clearkey-mp4-playback-retrieve-persistent-license.https.html
+
+        In the sync-message handler, a ref() was being taken waiting for a
+        CDM instance to be attached. This hits asserts since you are not
+        allowed to ref() an object created on the main thread
+        (BasePlayer) on a background thread.
+
+        The protection condition was overly scoped, tidied up the locking
+        and made it more granular. To avoid needing to hold a ref() in the
+        background thread, use instead a semaphore to signal when a CDM
+        instance is attached, or the player has been destroyed.
+
+        Also remove an erroneous safe-guard, the operator= in
+        isCDMInstanceAvailable will ref() the CDMInstance for us. This use
+        of holding a reference to CDMInstance in the decryptors is not
+        thread-safe, and now we have a problem since there's no clean way
+        to communicate with CDMInstance from background threads without
+        being thread unsafe. For ClearKey and Widevine, a thread safe
+        ProxyCDM needs to be designed and passed to background
+        threads (upcoming patch).
+
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
+        (WebCore::MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase):
+        (WebCore::MediaPlayerPrivateGStreamerBase::handleSyncMessage):
+        (WebCore::MediaPlayerPrivateGStreamerBase::cdmInstanceAttached):
+        (WebCore::MediaPlayerPrivateGStreamerBase::cdmInstanceDetached):
+        (WebCore::MediaPlayerPrivateGStreamerBase::handleProtectionEvent):
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:
+        * platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:
+        (isCDMInstanceAvailable):
+
 2019-08-30  Sihui Liu  <sihui_liu@apple.com>
 

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

@@ -245 +245 @@
 #endif
 
-#if ENABLE(ENCRYPTED_MEDIA)
-    m_protectionCondition.notifyAll();
-#endif
     m_notifier->invalidate();
 
@@ -266 +263 @@
     // waiting there, and ensure that any triggerRepaint call reaching the lock won't wait on m_drawCondition.
     cancelRepaint(true);
+
+#if ENABLE(ENCRYPTED_MEDIA)
+    m_cdmAttachmentSemaphore.signal();
+#endif
 
     // The change to GST_STATE_NULL state is always synchronous. So after this gets executed we don't need to worry
@@ -328 +329 @@
         }
         GST_DEBUG_OBJECT(pipeline(), "handling drm-preferred-decryption-system-id need context message");
-        LockHolder lock(m_protectionMutex);
-        ProtectionSystemEvents protectionSystemEvents(message);
-        GST_TRACE("found %zu protection events, %zu decryptors available", protectionSystemEvents.events().size(), protectionSystemEvents.availableSystems().size());
+
         InitData initData;
-
-        for (auto& event : protectionSystemEvents.events()) {
-            const char* eventKeySystemId = nullptr;
-            GstBuffer* data = nullptr;
-            gst_event_parse_protection(event.get(), &eventKeySystemId, &data, nullptr);
-
-            initData.append({eventKeySystemId, data});
-            m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event.get()));
+        {
+            LockHolder lock(m_protectionMutex);
+            ProtectionSystemEvents protectionSystemEvents(message);
+            GST_TRACE("found %zu protection events, %zu decryptors available", protectionSystemEvents.events().size(), protectionSystemEvents.availableSystems().size());
+
+            for (auto& event : protectionSystemEvents.events()) {
+                const char* eventKeySystemId = nullptr;
+                GstBuffer* data = nullptr;
+                gst_event_parse_protection(event.get(), &eventKeySystemId, &data, nullptr);
+
+                initData.append({eventKeySystemId, data});
+                m_handledProtectionEvents.add(GST_EVENT_SEQNUM(event.get()));
+            }
         }
-
         initializationDataEncountered(WTFMove(initData));
 
         GST_INFO_OBJECT(pipeline(), "waiting for a CDM instance");
-        m_protectionCondition.waitFor(m_protectionMutex, Seconds(4), [this] {
-            return this->m_cdmInstance;
-        });
-
-        if (m_cdmInstance && !m_cdmInstance->keySystem().isEmpty()) {
+        if (m_cdmAttachmentSemaphore.waitFor(4_s)
+            && m_notifier->isValid() // Check the player is not being destroyed.
+            && !m_cdmInstance->keySystem().isEmpty()) {
             const char* preferredKeySystemUuid = GStreamerEMEUtilities::keySystemToUuid(m_cdmInstance->keySystem());
             GST_INFO_OBJECT(pipeline(), "working with key system %s, continuing with key system %s on %s", m_cdmInstance->keySystem().utf8().data(), preferredKeySystemUuid, GST_MESSAGE_SRC_NAME(message));
@@ -1322 +1323 @@
     GST_DEBUG_OBJECT(m_pipeline.get(), "CDM instance %p dispatched as context", m_cdmInstance.get());
 
-    m_protectionCondition.notifyAll();
+    m_cdmAttachmentSemaphore.signal();
 }
 
@@ -1342 +1343 @@
     GRefPtr<GstContext> context = adoptGRef(gst_context_new("drm-cdm-instance", FALSE));
     gst_element_set_context(GST_ELEMENT(m_pipeline.get()), context.get());
-
-    m_protectionCondition.notifyAll();
 }
 
@@ -1361 +1360 @@
 void MediaPlayerPrivateGStreamerBase::handleProtectionEvent(GstEvent* event)
 {
-    if (m_handledProtectionEvents.contains(GST_EVENT_SEQNUM(event))) {
-        GST_DEBUG_OBJECT(pipeline(), "event %u already handled", GST_EVENT_SEQNUM(event));
-        return;
+    {
+        LockHolder lock(m_protectionMutex);
+        if (m_handledProtectionEvents.contains(GST_EVENT_SEQNUM(event))) {
+            GST_DEBUG_OBJECT(pipeline(), "event %u already handled", GST_EVENT_SEQNUM(event));
+            return;
+        }
     }
     GST_DEBUG_OBJECT(pipeline(), "handling event %u from MSE", GST_EVENT_SEQNUM(event));

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

@@ -37 +37 @@
 #include <wtf/RunLoop.h>
 #include <wtf/WeakPtr.h>
+#include <wtf/threads/BinarySemaphore.h>
 
 #if USE(GSTREAMER_GL)
@@ -301 +302 @@
 
 #if ENABLE(ENCRYPTED_MEDIA)
-    Lock m_protectionMutex;
-    Condition m_protectionCondition;
+    BinarySemaphore m_cdmAttachmentSemaphore;
     RefPtr<const CDMInstance> m_cdmInstance;
+
+    Lock m_protectionMutex; // Guards access to m_handledProtectionEvents.
     HashSet<uint32_t> m_handledProtectionEvents;
+
     bool m_waitingForKey { false };
 #endif

trunk/Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp

@@ -294 +294 @@
         if (context) {
             const GValue* value = gst_structure_get_value(gst_context_get_structure(context.get()), "cdm-instance");
-            // Capture the CDMInstance into a separate variable to avoid missing a refcount.
-            CDMInstance* instance = value ? reinterpret_cast<CDMInstance*>(g_value_get_pointer(value)) : nullptr;
-            // ... And force a refcount bump using operator=.
-            priv->cdmInstance = instance;
+            priv->cdmInstance = value ? reinterpret_cast<CDMInstance*>(g_value_get_pointer(value)) : nullptr;
             if (priv->cdmInstance)
                 GST_DEBUG_OBJECT(self, "received a new CDM instance %p, refcount %u", priv->cdmInstance.get(), priv->cdmInstance->refCount());