[webkit-changes] [264392] trunk/Source/WebCore

Wed, 15 Jul 2020 05:57:21 -0700

Title: [264392] trunk/Source/WebCore
Revision
264392
Author
[email protected]
Date
2020-07-15 05:56:23 -0700 (Wed, 15 Jul 2020)

Log Message

[MSE][GStreamer] Break circular reference between SourceBufferPrivateGStreamer and AppendPipeline
https://bugs.webkit.org/show_bug.cgi?id=214345

Reviewed by Xabier Rodriguez-Calvar.

SourceBufferPrivate is ref counted.

AppendPipeline is owned exclusively by SourceBufferPrivateGStreamer:
it's born and destroyed with it, managed by a never-moved unique_ptr.

AppendPipeline needs a reference to SourceBufferPrivateGStreamer to
notify it of essential events like samples having been parsed. This
used to be a Ref<>, thus creating a circular reference leak:

AppendPipeline is only destroyed in SourceBufferPrivateGStreamer
destructor. AppendPipeline holds ref counted reference to
SourceBufferPrivateGStreamer, therefore neither are destroyed.

This patch breaks the cycle by replacing the Ref<> in AppendPipeline
with a plain old reference. This is safe because
SourceBufferPrivateGStreamer owns, and therefore is alive at least
just as long as AppendPipeline.

As a consequence of not using Ref<>, the SourceBufferPrivateGStreamer
constructor does no longer need to relax the adoption requirements and
unique_ptr<AppendPipeline> can be replaced by a UniqueRef<>.

* platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::AppendPipeline::AppendPipeline):
(WebCore::AppendPipeline::handleErrorConditionFromStreamingThread):
(WebCore::AppendPipeline::handleStateChangeMessage):
(WebCore::AppendPipeline::handleEndOfAppend):
(WebCore::AppendPipeline::appsinkNewSample):
(WebCore::AppendPipeline::didReceiveInitializationSegment):
(WebCore::AppendPipeline::connectDemuxerSrcPadToAppsink):
* platform/graphics/gstreamer/mse/AppendPipeline.h:
(WebCore::AppendPipeline::sourceBufferPrivate):
* platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
(WebCore::MediaPlayerPrivateGStreamerMSE::trackDetected):
* platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
(WebCore::SourceBufferPrivateGStreamer::SourceBufferPrivateGStreamer):
* platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h:

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (264391 => 264392)


--- trunk/Source/WebCore/ChangeLog	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/ChangeLog	2020-07-15 12:56:23 UTC (rev 264392)
@@ -1,3 +1,48 @@
+2020-07-15  Alicia Boya García  <[email protected]>
+
+        [MSE][GStreamer] Break circular reference between SourceBufferPrivateGStreamer and AppendPipeline
+        https://bugs.webkit.org/show_bug.cgi?id=214345
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        SourceBufferPrivate is ref counted.
+
+        AppendPipeline is owned exclusively by SourceBufferPrivateGStreamer:
+        it's born and destroyed with it, managed by a never-moved unique_ptr.
+
+        AppendPipeline needs a reference to SourceBufferPrivateGStreamer to
+        notify it of essential events like samples having been parsed. This
+        used to be a Ref<>, thus creating a circular reference leak:
+
+        AppendPipeline is only destroyed in SourceBufferPrivateGStreamer
+        destructor. AppendPipeline holds ref counted reference to
+        SourceBufferPrivateGStreamer, therefore neither are destroyed.
+
+        This patch breaks the cycle by replacing the Ref<> in AppendPipeline
+        with a plain old reference. This is safe because
+        SourceBufferPrivateGStreamer owns, and therefore is alive at least
+        just as long as AppendPipeline.
+
+        As a consequence of not using Ref<>, the SourceBufferPrivateGStreamer
+        constructor does no longer need to relax the adoption requirements and
+        unique_ptr<AppendPipeline> can be replaced by a UniqueRef<>.
+
+        * platform/graphics/gstreamer/mse/AppendPipeline.cpp:
+        (WebCore::AppendPipeline::AppendPipeline):
+        (WebCore::AppendPipeline::handleErrorConditionFromStreamingThread):
+        (WebCore::AppendPipeline::handleStateChangeMessage):
+        (WebCore::AppendPipeline::handleEndOfAppend):
+        (WebCore::AppendPipeline::appsinkNewSample):
+        (WebCore::AppendPipeline::didReceiveInitializationSegment):
+        (WebCore::AppendPipeline::connectDemuxerSrcPadToAppsink):
+        * platform/graphics/gstreamer/mse/AppendPipeline.h:
+        (WebCore::AppendPipeline::sourceBufferPrivate):
+        * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
+        (WebCore::MediaPlayerPrivateGStreamerMSE::trackDetected):
+        * platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
+        (WebCore::SourceBufferPrivateGStreamer::SourceBufferPrivateGStreamer):
+        * platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h:
+
 2020-07-15  Brady Eidson  <[email protected]>
 
         Resolve race between IOHIDManager and GameController framework.

Modified: trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp (264391 => 264392)


--- trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp	2020-07-15 12:56:23 UTC (rev 264392)
@@ -99,8 +99,8 @@
     }
 }
 
-AppendPipeline::AppendPipeline(Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate, MediaPlayerPrivateGStreamerMSE& playerPrivate)
-    : m_sourceBufferPrivate(sourceBufferPrivate.get())
+AppendPipeline::AppendPipeline(SourceBufferPrivateGStreamer& sourceBufferPrivate, MediaPlayerPrivateGStreamerMSE& playerPrivate)
+    : m_sourceBufferPrivate(sourceBufferPrivate)
     , m_playerPrivate(&playerPrivate)
     , m_id(0)
     , m_wasBusAlreadyNotifiedOfAvailableSamples(false)
@@ -115,7 +115,7 @@
     // The track name is still unknown at this time, though.
     static size_t appendPipelineCount = 0;
     String pipelineName = makeString("append-pipeline-",
-        m_sourceBufferPrivate->type().containerType().replace("/", "-"), '-', appendPipelineCount++);
+        m_sourceBufferPrivate.type().containerType().replace("/", "-"), '-', appendPipelineCount++);
     m_pipeline = gst_pipeline_new(pipelineName.utf8().data());
 
     m_bus = adoptGRef(gst_pipeline_get_bus(GST_PIPELINE(m_pipeline.get())));
@@ -141,7 +141,7 @@
         return static_cast<AppendPipeline*>(userData)->appsrcEndOfAppendCheckerProbe(padProbeInfo);
     }, this, nullptr);
 
-    const String& type = m_sourceBufferPrivate->type().containerType();
+    const String& type = m_sourceBufferPrivate.type().containerType();
     GST_DEBUG("SourceBuffer containerType: %s", type.utf8().data());
     if (type.endsWith("mp4") || type.endsWith("aac"))
         m_demux = gst_element_factory_make("qtdemux", nullptr);
@@ -285,7 +285,7 @@
     auto response = m_taskQueue.enqueueTaskAndWait<AbortableTaskQueue::Void>([this]() {
         m_errorReceived = true;
         // appendParsingFailed() will cause resetParserState() to be called.
-        m_sourceBufferPrivate->appendParsingFailed();
+        m_sourceBufferPrivate.appendParsingFailed();
         return AbortableTaskQueue::Void();
     });
 #ifdef NDEBUG
@@ -342,7 +342,7 @@
     if (GST_MESSAGE_SRC(message) == reinterpret_cast<GstObject*>(m_pipeline.get())) {
         GstState currentState, newState;
         gst_message_parse_state_changed(message, &currentState, &newState, nullptr);
-        CString sourceBufferType = String(m_sourceBufferPrivate->type().raw())
+        CString sourceBufferType = String(m_sourceBufferPrivate.type().raw())
             .replace("/", "_").replace(" ", "_")
             .replace("\"", "").replace("\'", "").utf8();
         CString dotFileName = makeString("webkit-append-",
@@ -448,7 +448,7 @@
     ASSERT(isMainThread());
     consumeAppsinkAvailableSamples();
     GST_TRACE_OBJECT(m_pipeline.get(), "Notifying SourceBufferPrivate the append is complete");
-    sourceBufferPrivate()->didReceiveAllPendingSamples();
+    sourceBufferPrivate().didReceiveAllPendingSamples();
 }
 
 void AppendPipeline::appsinkNewSample(GRefPtr<GstSample>&& sample)
@@ -494,7 +494,7 @@
         mediaSample->extendToTheBeginning();
     }
 
-    m_sourceBufferPrivate->didReceiveSample(mediaSample.get());
+    m_sourceBufferPrivate.didReceiveSample(mediaSample.get());
 }
 
 void AppendPipeline::didReceiveInitializationSegment()
@@ -526,7 +526,7 @@
         break;
     }
 
-    m_sourceBufferPrivate->didReceiveInitializationSegment(initializationSegment);
+    m_sourceBufferPrivate.didReceiveInitializationSegment(initializationSegment);
 }
 
 AtomString AppendPipeline::trackId()
@@ -751,7 +751,7 @@
     ASSERT(isMainThread());
     GST_DEBUG("Connecting to appsink");
 
-    const String& type = m_sourceBufferPrivate->type().containerType();
+    const String& type = m_sourceBufferPrivate.type().containerType();
     if (type.endsWith("webm"))
         gst_pad_add_probe(demuxerSrcPad, GST_PAD_PROBE_TYPE_EVENT_DOWNSTREAM, matroskademuxForceSegmentStartToEqualZero, nullptr, nullptr);
 
@@ -789,7 +789,7 @@
 
         // appendParsingFailed() will immediately cause a resetParserState() which will stop demuxing, then the
         // AppendPipeline will be destroyed.
-        m_sourceBufferPrivate->appendParsingFailed();
+        m_sourceBufferPrivate.appendParsingFailed();
         return;
     default:
         GST_WARNING_OBJECT(m_pipeline.get(), "Pad has unknown track type, ignoring: %" GST_PTR_FORMAT, caps.get());

Modified: trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h (264391 => 264392)


--- trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h	2020-07-15 12:56:23 UTC (rev 264392)
@@ -46,12 +46,12 @@
 class AppendPipeline {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    AppendPipeline(Ref<SourceBufferPrivateGStreamer>, MediaPlayerPrivateGStreamerMSE&);
+    AppendPipeline(SourceBufferPrivateGStreamer&, MediaPlayerPrivateGStreamerMSE&);
     virtual ~AppendPipeline();
 
     void pushNewBuffer(GRefPtr<GstBuffer>&&);
     void resetParserState();
-    Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate() { return m_sourceBufferPrivate.get(); }
+    SourceBufferPrivateGStreamer& sourceBufferPrivate() { return m_sourceBufferPrivate; }
     GstCaps* appsinkCaps() { return m_appsinkCaps.get(); }
     RefPtr<WebCore::TrackPrivateBase> track() { return m_track; }
     MediaPlayerPrivateGStreamerMSE* playerPrivate() { return m_playerPrivate; }
@@ -106,7 +106,7 @@
     // Used only for asserting EOS events are only caused by demuxing errors.
     bool m_errorReceived { false };
 
-    Ref<SourceBufferPrivateGStreamer> m_sourceBufferPrivate;
+    SourceBufferPrivateGStreamer& m_sourceBufferPrivate;
     MediaPlayerPrivateGStreamerMSE* m_playerPrivate;
 
     // (m_mediaType, m_id) is unique.

Modified: trunk/Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp (264391 => 264392)


--- trunk/Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp	2020-07-15 12:56:23 UTC (rev 264392)
@@ -703,9 +703,9 @@
     }
 
     if (firstTrackDetected)
-        m_playbackPipeline->attachTrack(appendPipeline.sourceBufferPrivate(), newTrack, caps);
+        m_playbackPipeline->attachTrack(makeRef(appendPipeline.sourceBufferPrivate()), newTrack, caps);
     else
-        m_playbackPipeline->reattachTrack(appendPipeline.sourceBufferPrivate(), newTrack, caps);
+        m_playbackPipeline->reattachTrack(makeRef(appendPipeline.sourceBufferPrivate()), newTrack, caps);
 }
 
 void MediaPlayerPrivateGStreamerMSE::getSupportedTypes(HashSet<String, ASCIICaseInsensitiveHash>& types)

Modified: trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp (264391 => 264392)


--- trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp	2020-07-15 12:56:23 UTC (rev 264392)
@@ -59,13 +59,12 @@
     , m_mediaSource(mediaSource)
     , m_type(contentType)
     , m_playerPrivate(playerPrivate)
+    , m_appendPipeline(makeUniqueRef<AppendPipeline>(*this, playerPrivate))
 #if !RELEASE_LOG_DISABLED
     , m_logger(mediaSource->logger())
     , m_logIdentifier(mediaSource->nextSourceBufferLogIdentifier())
 #endif
 {
-    relaxAdoptionRequirement();
-    m_appendPipeline = WTF::makeUnique<AppendPipeline>(makeRef(*this), m_playerPrivate);
 }
 
 void SourceBufferPrivateGStreamer::setClient(SourceBufferPrivateClient* client)

Modified: trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h (264391 => 264392)


--- trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h	2020-07-15 10:18:40 UTC (rev 264391)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h	2020-07-15 12:56:23 UTC (rev 264392)
@@ -99,7 +99,7 @@
     MediaSourcePrivateGStreamer* m_mediaSource;
     ContentType m_type;
     MediaPlayerPrivateGStreamerMSE& m_playerPrivate;
-    std::unique_ptr<AppendPipeline> m_appendPipeline;
+    UniqueRef<AppendPipeline> m_appendPipeline;
     SourceBufferPrivateClient* m_sourceBufferPrivateClient { nullptr };
     bool m_isReadyForMoreSamples = true;
     bool m_notifyWhenReadyForMoreSamples = false;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to