<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[245118] releases/WebKitGTK/webkit-2.24/Source/WebCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/245118">245118</a></dd>
<dt>Author</dt> <dd>carlosgc@webkit.org</dd>
<dt>Date</dt> <dd>2019-05-09 02:38:25 -0700 (Thu, 09 May 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>Merge <a href="http://trac.webkit.org/projects/webkit/changeset/244584">r244584</a> - [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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#releasesWebKitGTKwebkit224SourceWebCoreChangeLog">releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog</a></li>
<li><a href="#releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerGStreamerCommoncpp">releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp</a></li>
<li><a href="#releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerMediaPlayerPrivateGStreamercpp">releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp</a></li>
<li><a href="#releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerMediaPlayerPrivateGStreamerh">releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="releasesWebKitGTKwebkit224SourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog (245117 => 245118)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog  2019-05-09 09:38:21 UTC (rev 245117)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog     2019-05-09 09:38:25 UTC (rev 245118)
</span><span class="lines">@@ -1,3 +1,30 @@
</span><ins>+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):
+
</ins><span class="cx"> 2019-04-10  Philippe Normand  <pnormand@igalia.com>
</span><span class="cx"> 
</span><span class="cx">         there is no vp8 support in youtube.com/html5 page with libwebkit2gtk 2.24 (MSE enabled)
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerGStreamerCommoncpp"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp (245117 => 245118)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp    2019-05-09 09:38:21 UTC (rev 245117)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp       2019-05-09 09:38:25 UTC (rev 245118)
</span><span class="lines">@@ -223,6 +223,13 @@
</span><span class="cx">     std::call_once(onceFlag, [options = WTFMove(options)] {
</span><span class="cx">         isGStreamerInitialized = false;
</span><span class="cx"> 
</span><ins>+        // 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.");
+
</ins><span class="cx"> #if ENABLE(VIDEO) || ENABLE(WEB_AUDIO)
</span><span class="cx">         Vector<String> parameters = options.valueOr(extractGStreamerOptionsFromCommandLine());
</span><span class="cx">         char** argv = g_new0(char*, parameters.size() + 2);
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerMediaPlayerPrivateGStreamercpp"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp (245117 => 245118)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp        2019-05-09 09:38:21 UTC (rev 245117)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp   2019-05-09 09:38:25 UTC (rev 245118)
</span><span class="lines">@@ -248,7 +248,7 @@
</span><span class="cx"> 
</span><span class="cx"> void MediaPlayerPrivateGStreamer::load(const String& urlString)
</span><span class="cx"> {
</span><del>-    loadFull(urlString, nullptr, String());
</del><ins>+    loadFull(urlString, String());
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> static void setSyncOnClock(GstElement *element, bool sync)
</span><span class="lines">@@ -273,8 +273,7 @@
</span><span class="cx">     setSyncOnClock(audioSink(), sync);
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const gchar* playbinName,
-    const String& pipelineName)
</del><ins>+void MediaPlayerPrivateGStreamer::loadFull(const String& urlString, const String& pipelineName)
</ins><span class="cx"> {
</span><span class="cx">     // FIXME: This method is still called even if supportsType() returned
</span><span class="cx">     // IsNotSupported. This would deserve more investigation but meanwhile make
</span><span class="lines">@@ -289,7 +288,7 @@
</span><span class="cx">         return;
</span><span class="cx"> 
</span><span class="cx">     if (!m_pipeline)
</span><del>-        createGSTPlayBin(isMediaSource() ? "playbin" : playbinName, pipelineName);
</del><ins>+        createGSTPlayBin(url, pipelineName);
</ins><span class="cx">     syncOnClock(true);
</span><span class="cx">     if (m_fillTimer.isActive())
</span><span class="cx">         m_fillTimer.stop();
</span><span class="lines">@@ -335,7 +334,7 @@
</span><span class="cx">         (stream.hasCaptureVideoSource() || stream.hasCaptureAudioSource()) ? "Local" : "Remote",
</span><span class="cx">         "_0x", hex(reinterpret_cast<uintptr_t>(this), Lowercase));
</span><span class="cx"> 
</span><del>-    loadFull(String("mediastream://") + stream.id(), "playbin3", pipelineName);
</del><ins>+    loadFull(String("mediastream://") + stream.id(), pipelineName);
</ins><span class="cx">     syncOnClock(false);
</span><span class="cx"> 
</span><span class="cx"> #if USE(GSTREAMER_GL)
</span><span class="lines">@@ -2412,21 +2411,25 @@
</span><span class="cx"> }
</span><span class="cx"> #endif
</span><span class="cx"> 
</span><del>-void MediaPlayerPrivateGStreamer::createGSTPlayBin(const gchar* playbinName, const String& pipelineName)
</del><ins>+void MediaPlayerPrivateGStreamer::createGSTPlayBin(const URL& url, const String& pipelineName)
</ins><span class="cx"> {
</span><ins>+    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
+
</ins><span class="cx">     if (m_pipeline) {
</span><del>-        if (!playbinName) {
-            GST_INFO_OBJECT(pipeline(), "Keeping same playbin as nothing forced");
-            return;
-        }
-
</del><span class="cx">         if (!g_strcmp0(GST_OBJECT_NAME(gst_element_get_factory(m_pipeline.get())), playbinName)) {
</span><span class="cx">             GST_INFO_OBJECT(pipeline(), "Already using %s", playbinName);
</span><span class="cx">             return;
</span><span class="cx">         }
</span><span class="cx"> 
</span><del>-        GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.",
-            playbinName);
</del><ins>+        GST_INFO_OBJECT(pipeline(), "Tearing down as we need to use %s now.", playbinName);
</ins><span class="cx">         changePipelineState(GST_STATE_NULL);
</span><span class="cx">         m_pipeline = nullptr;
</span><span class="cx">     }
</span><span class="lines">@@ -2433,16 +2436,6 @@
</span><span class="cx"> 
</span><span class="cx">     ASSERT(!m_pipeline);
</span><span class="cx"> 
</span><del>-#if GST_CHECK_VERSION(1, 10, 0)
-    if (g_getenv("USE_PLAYBIN3"))
-        playbinName = "playbin3";
-#else
-    playbinName = "playbin";
-#endif
-
-    if (!playbinName)
-        playbinName = "playbin";
-
</del><span class="cx">     m_isLegacyPlaybin = !g_strcmp0(playbinName, "playbin");
</span><span class="cx"> 
</span><span class="cx">     // gst_element_factory_make() returns a floating reference so
</span></span></pre></div>
<a id="releasesWebKitGTKwebkit224SourceWebCoreplatformgraphicsgstreamerMediaPlayerPrivateGStreamerh"></a>
<div class="modfile"><h4>Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h (245117 => 245118)</h4>
<pre class="diff"><span>
<span class="info">--- releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h  2019-05-09 09:38:21 UTC (rev 245117)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h     2019-05-09 09:38:25 UTC (rev 245118)
</span><span class="lines">@@ -149,7 +149,7 @@
</span><span class="cx">     virtual void updateStates();
</span><span class="cx">     virtual void asyncStateChangeDone();
</span><span class="cx"> 
</span><del>-    void createGSTPlayBin(const gchar* playbinName, const String& pipelineName);
</del><ins>+    void createGSTPlayBin(const URL&, const String& pipelineName);
</ins><span class="cx"> 
</span><span class="cx">     bool loadNextLocation();
</span><span class="cx">     void mediaLocationChanged(GstMessage*);
</span><span class="lines">@@ -180,7 +180,7 @@
</span><span class="cx">     static void downloadBufferFileCreatedCallback(MediaPlayerPrivateGStreamer*);
</span><span class="cx"> 
</span><span class="cx">     void setPlaybinURL(const URL& urlString);
</span><del>-    void loadFull(const String& url, const gchar* playbinName, const String& pipelineName);
</del><ins>+    void loadFull(const String& url, const String& pipelineName);
</ins><span class="cx"> 
</span><span class="cx"> #if GST_CHECK_VERSION(1, 10, 0)
</span><span class="cx">     void updateTracks();
</span></span></pre>
</div>
</div>

</body>
</html>