Changeset 238412 in webkit

Timestamp:

Nov 21, 2018 12:49:39 AM (5 years ago)

Author:

aboya@igalia.com

Message:

[MSE][GStreamer] Refactor AppendPipeline deinitialization
​https://bugs.webkit.org/show_bug.cgi?id=191759

Reviewed by Xabier Rodriguez-Calvar.

AppendPipeline currently has a method, clearPlayerPrivate(), that the
client code uses to deinitialize most of the AppendPipeline... just
before actually destructing it in the next line of code.

Since at that point there should not be alive RefPtr's pointing to the
AppendPipeline there is no need for this kind of pre-deinitialization
in this usage pattern. Instead, we can just rely on C++ destructors,
cleaning the code a bit and removing the potential for the question
"what if clearPlayerPrivate() has been called before?": it has not.

Assertions have been added to ensure that there is only one alive
RefPtr pointing to AppendPipeline, therefore guaranteeing its immediate
destruction.

• platform/graphics/gstreamer/mse/AppendPipeline.cpp:

(WebCore::AppendPipeline::~AppendPipeline):
(WebCore::AppendPipeline::deinitialize):
(WebCore::AppendPipeline::clearPlayerPrivate): Deleted.

• platform/graphics/gstreamer/mse/AppendPipeline.h:
• platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:

(WebCore::MediaPlayerPrivateGStreamerMSE::~MediaPlayerPrivateGStreamerMSE):

• platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:

(WebCore::MediaSourceClientGStreamerMSE::removedFromMediaSource):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/gstreamer/mse/AppendPipeline.cpp (modified) (6 diffs)
• platform/graphics/gstreamer/mse/AppendPipeline.h (modified) (1 diff)
• platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp (modified) (1 diff)
• platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-11-21  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Refactor AppendPipeline deinitialization
+        https://bugs.webkit.org/show_bug.cgi?id=191759
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        AppendPipeline currently has a method, clearPlayerPrivate(), that the
+        client code uses to deinitialize most of the AppendPipeline... just
+        before actually destructing it in the next line of code.
+
+        Since at that point there should not be alive RefPtr's pointing to the
+        AppendPipeline there is no need for this kind of pre-deinitialization
+        in this usage pattern. Instead, we can just rely on C++ destructors,
+        cleaning the code a bit and removing the potential for the question
+        "what if `clearPlayerPrivate() has been called before?`": it has not.
+
+        Assertions have been added to ensure that there is only one alive
+        RefPtr pointing to AppendPipeline, therefore guaranteeing its immediate
+        destruction.
+
+        * platform/graphics/gstreamer/mse/AppendPipeline.cpp:
+        (WebCore::AppendPipeline::~AppendPipeline):
+        (WebCore::AppendPipeline::deinitialize):
+        (WebCore::AppendPipeline::clearPlayerPrivate): Deleted.
+        * platform/graphics/gstreamer/mse/AppendPipeline.h:
+        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
+        (WebCore::MediaPlayerPrivateGStreamerMSE::~MediaPlayerPrivateGStreamerMSE):
+        * platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:
+        (WebCore::MediaSourceClientGStreamerMSE::removedFromMediaSource):
+
 2018-11-20  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp

@@ -226 +226 @@
 AppendPipeline::~AppendPipeline()
 {
+    GST_TRACE("Destructing AppendPipeline (%p)", this);
     ASSERT(isMainThread());
 
@@ -232 +233 @@
     m_taskQueue.startAborting();
 
-    GST_TRACE("Destroying AppendPipeline (%p)", this);
-
-    // FIXME: Maybe notify appendComplete here?
+    // Disconnect all synchronous event handlers and probes susceptible of firing from the main thread
+    // when changing the pipeline state.
 
     if (m_pipeline) {
@@ -241 +241 @@
         gst_bus_disable_sync_message_emission(m_bus.get());
         gst_bus_remove_signal_watch(m_bus.get());
-        gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
-        m_pipeline = nullptr;
-    }
-
-    if (m_appsrc) {
+    }
+
+    if (m_appsrc)
         g_signal_handlers_disconnect_by_data(m_appsrc.get(), this);
-        m_appsrc = nullptr;
-    }
 
     if (m_demux) {
@@ -257 +253 @@
 
         g_signal_handlers_disconnect_by_data(m_demux.get(), this);
-        m_demux = nullptr;
     }
 
@@ -272 +267 @@
         gst_pad_remove_probe(appsinkPad.get(), m_appsinkPadEventProbeInformation.probeId);
 #endif
-        m_appsink = nullptr;
-    }
-
-    m_appsinkCaps = nullptr;
-    m_demuxerSrcPadCaps = nullptr;
-};
+    }
+
+    // We can tear down the pipeline safely now.
+    if (m_pipeline)
+        gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
+}
 
 GstPadProbeReturn AppendPipeline::appsrcEndOfAppendCheckerProbe(GstPadProbeInfo* padProbeInfo)
@@ -298 +293 @@
     });
     return GST_PAD_PROBE_DROP;
-}
-
-void AppendPipeline::clearPlayerPrivate()
-{
-    ASSERT(isMainThread());
-    GST_DEBUG("cleaning private player");
-
-    // Make sure that AppendPipeline won't process more data from now on and
-    // instruct handleNewSample to abort itself from now on as well.
-    setAppendState(AppendState::Invalid);
-    m_taskQueue.startAborting();
-
-    // And now that no handleNewSample operations will remain stalled waiting
-    // for the main thread, stop the pipeline.
-    if (m_pipeline)
-        gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
 }
 

trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h

@@ -51 +51 @@
 
     GstFlowReturn pushNewBuffer(GstBuffer*);
-    void clearPlayerPrivate();
     void abort();
     Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate() { return m_sourceBufferPrivate.get(); }

trunk/Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp

@@ -98 +98 @@
     GST_TRACE("destroying the player (%p)", this);
 
+    // Clear the AppendPipeline map. This should cause the destruction of all the AppendPipeline's since there should
+    // be no alive references at this point.
+#ifndef NDEBUG
     for (auto iterator : m_appendPipelinesMap)
-        iterator.value->clearPlayerPrivate();
+        ASSERT(iterator.value->hasOneRef());
+#endif
+    m_appendPipelinesMap.clear();
 
     if (m_source) {

trunk/Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp

@@ -170 +170 @@
     ASSERT(m_playerPrivate->m_playbackPipeline);
 
-    RefPtr<AppendPipeline> appendPipeline = m_playerPrivate->m_appendPipelinesMap.get(sourceBufferPrivate);
-
-    ASSERT(appendPipeline);
-
-    appendPipeline->clearPlayerPrivate();
+    // Remove the AppendPipeline from the map. This should cause its destruction since there should be no alive
+    // references at this point.
+    ASSERT(m_playerPrivate->m_appendPipelinesMap.get(sourceBufferPrivate)->hasOneRef());
     m_playerPrivate->m_appendPipelinesMap.remove(sourceBufferPrivate);
-    // AppendPipeline destructor will take care of cleaning up when appropriate.
 
     m_playerPrivate->m_playbackPipeline->removeSourceBuffer(sourceBufferPrivate);