[GStreamer] Crash in AudioTrackPrivate with playbin3 enabled
authorphiln@webkit.org <philn@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Apr 2019 12:11:50 +0000 (12:11 +0000)
committerphiln@webkit.org <philn@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 24 Apr 2019 12:11:50 +0000 (12:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196913

Reviewed by Xabier Rodriguez-Calvar.

The crash was due to a playbin3 code path being triggered during
MSE playback, which is not supposed to work in playbin3 anyway.
The problem is that setting the USE_PLAYBIN3 environment variable
to "1" makes the GStreamer playback plugin register the playbin3
element under the playbin name. So that leads to playbin3 being
used everywhere in WebKit where we assume the playbin element is
used. So the proposed solution is to:

- use a WebKit-specific environment variable instead of the
GStreamer USE_PLAYBIN3 variable.
- emit a warning if the USE_PLAYBIN3 environment variable is
detected. We can't unset it ourselves for security reasons.

The patch also includes a code cleanup of the player method
handling the pipeline creation. The previous code had a bug
leading to playbin3 being used for MSE.

* platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244584 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h

index 16abd53..68caca7 100644 (file)
@@ -1,3 +1,30 @@
+2019-04-24  Philippe Normand  <pnormand@igalia.com>
+
+        [GStreamer] Crash in AudioTrackPrivate with playbin3 enabled
+        https://bugs.webkit.org/show_bug.cgi?id=196913
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        The crash was due to a playbin3 code path being triggered during
+        MSE playback, which is not supposed to work in playbin3 anyway.
+        The problem is that setting the USE_PLAYBIN3 environment variable
+        to "1" makes the GStreamer playback plugin register the playbin3
+        element under the playbin name. So that leads to playbin3 being
+        used everywhere in WebKit where we assume the playbin element is
+        used. So the proposed solution is to:
+
+        - use a WebKit-specific environment variable instead of the
+        GStreamer USE_PLAYBIN3 variable.
+        - emit a warning if the USE_PLAYBIN3 environment variable is
+        detected. We can't unset it ourselves for security reasons.
+
+        The patch also includes a code cleanup of the player method
+        handling the pipeline creation. The previous code had a bug
+        leading to playbin3 being used for MSE.
+
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
+        (WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin):
+
 2019-04-24  chris fleizach  <cfleizach@apple.com>
 
         AX: Remove deprecated Accessibility Object Model events
index db3563a..ce38d50 100644 (file)
@@ -223,6 +223,13 @@ bool initializeGStreamer(Optional<Vector<String>>&& options)
     std::call_once(onceFlag, [options = WTFMove(options)] {
         isGStreamerInitialized = false;
 
+        // USE_PLAYBIN3 is dangerous for us because its potential sneaky effect
+        // is to register the playbin3 element under the playbin namespace. We
+        // can't allow this, when we create playbin, we want playbin2, not
+        // playbin3.
+        if (g_getenv("USE_PLAYBIN3"))
+            WTFLogAlways("The USE_PLAYBIN3 variable was detected in the environment. Expect playback issues or please unset it.");
+
 #if ENABLE(VIDEO) || ENABLE(WEB_AUDIO)
         Vector<String> parameters = options.valueOr(extractGStreamerOptionsFromCommandLine());
         char** argv = g_new0(char*, parameters.size() + 2);
index 3f5263d..b849f6b 100644 (file)
@@ -248,7 +248,7 @@ void MediaPlayerPrivateGStreamer::setPlaybinURL(const URL& url)
 
 void MediaPlayerPrivateGStreamer::load(const String& urlString)
 {
-    loadFull(urlString, nullptr, String());
+    loadFull(urlString, String());
 }
 
 static void setSyncOnClock(GstElement *element, bool sync)
@@ -273,8 +273,7 @@ void MediaPlayerPrivateGStreamer::syncOnClock(bool sync)
     setSyncOnClock(audioSink(), sync);
 }
 
-void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const gchar* playbinName,
-    const String& pipelineName)
+void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const String& pipelineName)
 {
     // FIXME: This method is still called even if supportsType() returned
     // IsNotSupported. This would deserve more investigation but meanwhile make
@@ -289,7 +288,7 @@ void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const gchar*
         return;
 
     if (!m_pipeline)
-        createGSTPlayBin(isMediaSource() ? "playbin" : playbinName, pipelineName);
+        createGSTPlayBin(url, pipelineName);
     syncOnClock(true);
     if (m_fillTimer.isActive())
         m_fillTimer.stop();
@@ -335,7 +334,7 @@ void MediaPlayerPrivateGStreamer::load(MediaStreamPrivate& stream)
         (stream.hasCaptureVideoSource() || stream.hasCaptureAudioSource()) ? "Local" : "Remote",
         "_0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase));
 
-    loadFull(String("mediastream://") + stream.id(), "playbin3", pipelineName);
+    loadFull(String("mediastream://") + stream.id(), pipelineName);
     syncOnClock(false);
 
 #if USE(GSTREAMER_GL)
@@ -2362,37 +2361,31 @@ AudioSourceProvider* MediaPlayerPrivateGStreamer::audioSourceProvider()
 }
 #endif
 
-void MediaPlayerPrivateGStreamer::createGSTPlayBin(const gchar* playbinName, const String& pipelineName)
+void MediaPlayerPrivateGStreamer::createGSTPlayBin(const URL& url, const String& pipelineName)
 {
-    if (m_pipeline) {
-        if (!playbinName) {
-            GST_INFO_OBJECT(pipeline(), "Keeping same playbin as nothing forced");
-            return;
-        }
+    const gchar* playbinName = "playbin";
+
+    // MSE doesn't support playbin3. Mediastream requires playbin3. Regular
+    // playback can use playbin3 on-demand with the WEBKIT_GST_USE_PLAYBIN3
+    // environment variable.
+#if GST_CHECK_VERSION(1, 10, 0)
+    if ((!isMediaSource() && g_getenv("WEBKIT_GST_USE_PLAYBIN3")) || url.protocolIs("mediastream"))
+        playbinName = "playbin3";
+#endif
 
+    if (m_pipeline) {
         if (!g_strcmp0(GST_OBJECT_NAME(gst_element_get_factory(m_pipeline.get())), playbinName)) {
             GST_INFO_OBJECT(pipeline(), "Already using %s", playbinName);
             return;
         }
 
-        GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.",
-            playbinName);
+        GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.", playbinName);
         changePipelineState(GST_STATE_NULL);
         m_pipeline = nullptr;
     }
 
     ASSERT(!m_pipeline);
 
-#if GST_CHECK_VERSION(1, 10, 0)
-    if (g_getenv("USE_PLAYBIN3"))
-        playbinName = "playbin3";
-#else
-    playbinName = "playbin";
-#endif
-
-    if (!playbinName)
-        playbinName = "playbin";
-
     m_isLegacyPlaybin = !g_strcmp0(playbinName, "playbin");
 
     // gst_element_factory_make() returns a floating reference so
index 1f574b2..82db800 100644 (file)
@@ -149,7 +149,7 @@ private:
     virtual void updateStates();
     virtual void asyncStateChangeDone();
 
-    void createGSTPlayBin(const gchar* playbinName, const String& pipelineName);
+    void createGSTPlayBin(const URL&, const String& pipelineName);
 
     bool loadNextLocation();
     void mediaLocationChanged(GstMessage*);
@@ -180,7 +180,7 @@ private:
     static void downloadBufferFileCreatedCallback(MediaPlayerPrivateGStreamer*);
 
     void setPlaybinURL(const URL& urlString);
-    void loadFull(const String& url, const gchar* playbinName, const String& pipelineName);
+    void loadFull(const String& url, const String& pipelineName);
 
 #if GST_CHECK_VERSION(1, 10, 0)
     void updateTracks();