[GStreamer] vid.me videos do not play
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 10:12:49 +0000 (10:12 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 10:12:49 +0000 (10:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172240

Patch by Charlie Turner <cturner@igalia.com> on 2017-07-06
Reviewed by Xabier Rodriguez-Calvar.

Source/WebCore:

In r142251, code to hide the WK HTTP source elements from elsewhere in
the pipeline was removed. This has the nasty side-effect of
auto-plugging the WK HTTP source into things it really should not be
used in, especially the adaptive streaming demuxers. The reasons this
is bad are documented in several places on Bugzilla, see the parent
bug report for more details. The high-level issue is that the WK HTTP
source and its use of WebCore is not thread-safe. Although work has
been recently done to improve this situation, it's still not perfect.

Another issue is the interface hlsdemux expects its HTTP source to
implement, specifically seeking in READY.

This does rely on HTTP context sharing being available in GStreamer,
upstream bug is here:
https://bugzilla.gnome.org/show_bug.cgi?id=761099. The failing case
can be demonstrated with
https://github.com/thiagoss/adaptive-test-server but manual testing on
popular video hosting sites, including vid.me, shows that this doesn't
bite us at the moment, just something else to fix in the future.

There are some QoS issues with the adaptive streaming code in
GStreamer, but it seems much better to offer a below par QoS in lieu
of crashing/livelocking when playing certain streams, and issues can be
raised upstream when they arise.

This patch does take us further away from the future goal of having all
networking operations go through the network process, but in return it
solves some nasty crashes and livelocks that have been irritating
users for some time. With the pressure off on this issue, work can be
planned to consider how to make the WK HTTP source a better citizen
inside the GStreamer pipeline when we migrate the netcode to go
through the network process.

A new test is added to check that the single file HLS playlists
(new in version 4) can be played, which was the primary cause of
this bug report.

Test: http/tests/media/hls/range-request.html

* platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
(WebCore::MediaPlayerPrivateGStreamer::setPlaybinURL): Perform
some trickery to make sure that we only ever fetch URLs handed to
us by WebCore. Any further URLs discovered inside the pipeline
will not get WKWS auto-plugged, since they'll be plain https?
schemas.
(WebCore::MediaPlayerPrivateGStreamer::load): Refactor to use the
setPlaybinURL helper method.
(WebCore::MediaPlayerPrivateGStreamer::loadNextLocation): Ditto.
* platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Add
the setPlaybinURL helper method.
* platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
(webKitWebSrcGetProtocols): Only advertise webkit+https?, this
ensures we won't get auto-plugged by pipeline elements asking for
an element to fetch https? resources (like adaptive demuxers).
(convertPlaybinURI): Undo the trick when another element asks us
for our URI.

Tools:

Build httpsoupsrc again for use in adaptive streaming pipelines, and
have the existing libsoup build against GNOME to avoid header drift
against GStreamer's linked Soup library.

* gtk/jhbuild.modules:

LayoutTests:

Add a test for single output file HLS playlists that require HTTP
range requests to playback. This failed using the WK http source
for reasons documented in the linked bug.

Generated with mp4hls --segment-duration 3 --output-single-file

* Http/tests/media/hls/range-request-expected.txt: Added.
* http/tests/media/hls/range-request.html: Added.
* http/tests/media/resources/hls/range-request-playlist.m3u8: Added.
* http/tests/media/resources/hls/range-request-playlists/iframes.m3u8: Added.
* http/tests/media/resources/hls/range-request-playlists/media.ts: Added.
* http/tests/media/resources/hls/range-request-playlists/stream.m3u8: Added.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/media/hls/range-request-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/media/hls/range-request.html [new file with mode: 0644]
LayoutTests/http/tests/media/resources/hls/range-request-playlist.m3u8 [new file with mode: 0644]
LayoutTests/http/tests/media/resources/hls/range-request-playlists/iframes.m3u8 [new file with mode: 0644]
LayoutTests/http/tests/media/resources/hls/range-request-playlists/media.ts [new file with mode: 0644]
LayoutTests/http/tests/media/resources/hls/range-request-playlists/stream.m3u8 [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
Tools/ChangeLog
Tools/gtk/jhbuild.modules

index b80e06b..47a5b07 100644 (file)
@@ -1,3 +1,23 @@
+2017-07-06  Charlie Turner  <cturner@igalia.com>
+
+        [GStreamer] vid.me videos do not play
+        https://bugs.webkit.org/show_bug.cgi?id=172240
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Add a test for single output file HLS playlists that require HTTP
+        range requests to playback. This failed using the WK http source
+        for reasons documented in the linked bug.
+
+        Generated with mp4hls --segment-duration 3 --output-single-file
+
+        * Http/tests/media/hls/range-request-expected.txt: Added.
+        * http/tests/media/hls/range-request.html: Added.
+        * http/tests/media/resources/hls/range-request-playlist.m3u8: Added.
+        * http/tests/media/resources/hls/range-request-playlists/iframes.m3u8: Added.
+        * http/tests/media/resources/hls/range-request-playlists/media.ts: Added.
+        * http/tests/media/resources/hls/range-request-playlists/stream.m3u8: Added.
+
 2017-07-05  Zalan Bujtas  <zalan@apple.com>
 
         REGRESSION: Stack overflow in RenderBlockFlow::layoutBlock after increasing the font size to max in some RTL vertical books.
diff --git a/LayoutTests/http/tests/media/hls/range-request-expected.txt b/LayoutTests/http/tests/media/hls/range-request-expected.txt
new file mode 100644 (file)
index 0000000..776c6dd
--- /dev/null
@@ -0,0 +1,3 @@
+
+EVENT(playing)
+
diff --git a/LayoutTests/http/tests/media/hls/range-request.html b/LayoutTests/http/tests/media/hls/range-request.html
new file mode 100644 (file)
index 0000000..12ba34f
--- /dev/null
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+    <head>
+        <script src=../../media-resources/video-test.js></script>
+        <script src=../../media-resources/media-controls.js></script>
+        <script>
+            if (window.testRunner) {
+                testRunner.dumpAsText();
+                testRunner.setAlwaysAcceptCookies(true);
+                testRunner.waitUntilDone();
+            }
+
+            function playing() {
+                testRunner.notifyDone();
+            }
+
+            function start() {
+                video = document.getElementById('video');
+                video.autoplay = true
+                waitForEvent("playing", playing);
+                video.src = "../resources/hls/range-request-playlist.m3u8";
+            }
+        </script>
+    </head>
+    <body onload="start()">
+        <video id="video"></video>
+    </body>
+</html>
diff --git a/LayoutTests/http/tests/media/resources/hls/range-request-playlist.m3u8 b/LayoutTests/http/tests/media/resources/hls/range-request-playlist.m3u8
new file mode 100644 (file)
index 0000000..64b65bd
--- /dev/null
@@ -0,0 +1,11 @@
+#EXTM3U
+# Created with Bento4 mp4-hls.py version 1.1.0r615
+
+#EXT-X-VERSION:4
+
+# Media Playlists
+#EXT-X-STREAM-INF:AVERAGE-BANDWIDTH=361262,BANDWIDTH=368806,CODECS="avc1.42C00D",RESOLUTION=640x480
+range-request-playlists/stream.m3u8
+
+# I-Frame Playlists
+#EXT-X-I-FRAME-STREAM-INF:AVERAGE-BANDWIDTH=179875,BANDWIDTH=195520,CODECS="avc1.42C00D",RESOLUTION=640x480,URI="range-request-playlists/iframes.m3u8"
diff --git a/LayoutTests/http/tests/media/resources/hls/range-request-playlists/iframes.m3u8 b/LayoutTests/http/tests/media/resources/hls/range-request-playlists/iframes.m3u8
new file mode 100644 (file)
index 0000000..06cb560
--- /dev/null
@@ -0,0 +1,38 @@
+#EXTM3U
+#EXT-X-VERSION:4
+#EXT-X-PLAYLIST-TYPE:VOD
+#EXT-X-I-FRAMES-ONLY
+#EXT-X-INDEPENDENT-SEGMENTS
+#EXT-X-TARGETDURATION:1
+#EXT-X-MEDIA-SEQUENCE:0
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:21808@376
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:20680@44180
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:24440@87044
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:21056@134044
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:24252@177848
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:21056@224284
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:24252@268464
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:21056@315088
+media.ts
+#EXTINF:1.000000,
+#EXT-X-BYTERANGE:24252@358892
+media.ts
+#EXTINF:0.958333,
+#EXT-X-BYTERANGE:21056@405892
+media.ts
+#EXT-X-ENDLIST
diff --git a/LayoutTests/http/tests/media/resources/hls/range-request-playlists/media.ts b/LayoutTests/http/tests/media/resources/hls/range-request-playlists/media.ts
new file mode 100644 (file)
index 0000000..f798d6b
Binary files /dev/null and b/LayoutTests/http/tests/media/resources/hls/range-request-playlists/media.ts differ
diff --git a/LayoutTests/http/tests/media/resources/hls/range-request-playlists/stream.m3u8 b/LayoutTests/http/tests/media/resources/hls/range-request-playlists/stream.m3u8
new file mode 100644 (file)
index 0000000..58742d6
--- /dev/null
@@ -0,0 +1,19 @@
+#EXTM3U
+#EXT-X-VERSION:4
+#EXT-X-PLAYLIST-TYPE:VOD
+#EXT-X-INDEPENDENT-SEGMENTS
+#EXT-X-TARGETDURATION:3
+#EXT-X-MEDIA-SEQUENCE:0
+#EXTINF:3.000000,
+#EXT-X-BYTERANGE:133668@0
+media.ts
+#EXTINF:3.000000,
+#EXT-X-BYTERANGE:134420@133668
+media.ts
+#EXTINF:3.000000,
+#EXT-X-BYTERANGE:137428@268088
+media.ts
+#EXTINF:0.958333,
+#EXT-X-BYTERANGE:44180@405516
+media.ts
+#EXT-X-ENDLIST
index 03e231e..c578cee 100644 (file)
@@ -1,3 +1,67 @@
+2017-07-06  Charlie Turner  <cturner@igalia.com>
+
+        [GStreamer] vid.me videos do not play
+        https://bugs.webkit.org/show_bug.cgi?id=172240
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        In r142251, code to hide the WK HTTP source elements from elsewhere in
+        the pipeline was removed. This has the nasty side-effect of
+        auto-plugging the WK HTTP source into things it really should not be
+        used in, especially the adaptive streaming demuxers. The reasons this
+        is bad are documented in several places on Bugzilla, see the parent
+        bug report for more details. The high-level issue is that the WK HTTP
+        source and its use of WebCore is not thread-safe. Although work has
+        been recently done to improve this situation, it's still not perfect.
+
+        Another issue is the interface hlsdemux expects its HTTP source to
+        implement, specifically seeking in READY.
+
+        This does rely on HTTP context sharing being available in GStreamer,
+        upstream bug is here:
+        https://bugzilla.gnome.org/show_bug.cgi?id=761099. The failing case
+        can be demonstrated with
+        https://github.com/thiagoss/adaptive-test-server but manual testing on
+        popular video hosting sites, including vid.me, shows that this doesn't
+        bite us at the moment, just something else to fix in the future.
+
+        There are some QoS issues with the adaptive streaming code in
+        GStreamer, but it seems much better to offer a below par QoS in lieu
+        of crashing/livelocking when playing certain streams, and issues can be
+        raised upstream when they arise.
+
+        This patch does take us further away from the future goal of having all
+        networking operations go through the network process, but in return it
+        solves some nasty crashes and livelocks that have been irritating
+        users for some time. With the pressure off on this issue, work can be
+        planned to consider how to make the WK HTTP source a better citizen
+        inside the GStreamer pipeline when we migrate the netcode to go
+        through the network process.
+
+        A new test is added to check that the single file HLS playlists
+        (new in version 4) can be played, which was the primary cause of
+        this bug report.
+
+        Test: http/tests/media/hls/range-request.html
+
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
+        (WebCore::MediaPlayerPrivateGStreamer::setPlaybinURL): Perform
+        some trickery to make sure that we only ever fetch URLs handed to
+        us by WebCore. Any further URLs discovered inside the pipeline
+        will not get WKWS auto-plugged, since they'll be plain https?
+        schemas.
+        (WebCore::MediaPlayerPrivateGStreamer::load): Refactor to use the
+        setPlaybinURL helper method.
+        (WebCore::MediaPlayerPrivateGStreamer::loadNextLocation): Ditto.
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Add
+        the setPlaybinURL helper method.
+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
+        (webKitWebSrcGetProtocols): Only advertise webkit+https?, this
+        ensures we won't get auto-plugged by pipeline elements asking for
+        an element to fetch https? resources (like adaptive demuxers).
+        (convertPlaybinURI): Undo the trick when another element asks us
+        for our URI.
+
 2017-05-24  Sergio Villar Senin  <svillar@igalia.com>
 
         [SVG] Leak in SVGAnimatedListPropertyTearOff
index dd48e32..119ce68 100644 (file)
@@ -210,6 +210,22 @@ MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer()
     }
 }
 
+void MediaPlayerPrivateGStreamer::setPlaybinURL(const URL& url)
+{
+    // Clean out everything after file:// url path.
+    String cleanURLString(url.string());
+    if (url.isLocalFile())
+        cleanURLString = cleanURLString.substring(0, url.pathEnd());
+
+    m_url = URL(URL(), cleanURLString);
+
+    if (m_url.protocolIsInHTTPFamily())
+        m_url.setProtocol("webkit+" + url.protocol());
+
+    GST_INFO("Load %s", cleanURLString.utf8().data());
+    g_object_set(m_pipeline.get(), "uri", cleanURLString.utf8().data(), nullptr);
+}
+
 void MediaPlayerPrivateGStreamer::load(const String& urlString)
 {
     if (!MediaPlayerPrivateGStreamerBase::initializeGStreamerAndRegisterWebKitElements())
@@ -219,11 +235,6 @@ void MediaPlayerPrivateGStreamer::load(const String& urlString)
     if (url.isBlankURL())
         return;
 
-    // Clean out everything after file:// url path.
-    String cleanURL(urlString);
-    if (url.isLocalFile())
-        cleanURL = cleanURL.substring(0, url.pathEnd());
-
     if (!m_pipeline)
         createGSTPlayBin();
 
@@ -232,10 +243,7 @@ void MediaPlayerPrivateGStreamer::load(const String& urlString)
 
     ASSERT(m_pipeline);
 
-    m_url = URL(URL(), cleanURL);
-    g_object_set(m_pipeline.get(), "uri", cleanURL.utf8().data(), nullptr);
-
-    GST_INFO("Load %s", cleanURL.utf8().data());
+    setPlaybinURL(url);
 
     if (m_preload == MediaPlayer::None) {
         GST_DEBUG("Delaying load.");
@@ -1679,8 +1687,7 @@ bool MediaPlayerPrivateGStreamer::loadNextLocation()
             gst_element_get_state(m_pipeline.get(), &state, nullptr, 0);
             if (state <= GST_STATE_READY) {
                 // Set the new uri and start playing.
-                g_object_set(m_pipeline.get(), "uri", newUrl.string().utf8().data(), nullptr);
-                m_url = newUrl;
+                setPlaybinURL(newUrl);
                 changePipelineState(GST_STATE_PLAYING);
                 return true;
             }
index 40b20e9..3a30a23 100644 (file)
@@ -173,6 +173,8 @@ private:
     static void uriDecodeBinElementAddedCallback(GstBin*, GstElement*, MediaPlayerPrivateGStreamer*);
     static void downloadBufferFileCreatedCallback(MediaPlayerPrivateGStreamer*);
 
+    void setPlaybinURL(const URL& urlString);
+
 protected:
     void cacheDuration();
 
index 7b29954..c06d226 100644 (file)
@@ -727,10 +727,18 @@ static GstURIType webKitWebSrcUriGetType(GType)
 
 const gchar* const* webKitWebSrcGetProtocols(GType)
 {
-    static const char* protocols[] = {"http", "https", "blob", nullptr };
+    static const char* protocols[] = {"webkit+http", "webkit+https", "blob", nullptr };
     return protocols;
 }
 
+static URL convertPlaybinURI(const char* uriString)
+{
+    URL url(URL(), uriString);
+    ASSERT(url.protocol().substring(0, 7) == "webkit+");
+    url.setProtocol(url.protocol().substring(7).toString());
+    return url;
+}
+
 static gchar* webKitWebSrcGetUri(GstURIHandler* handler)
 {
     WebKitWebSrc* src = WEBKIT_WEB_SRC(handler);
@@ -758,7 +766,7 @@ static gboolean webKitWebSrcSetUri(GstURIHandler* handler, const gchar* uri, GEr
     if (!uri)
         return TRUE;
 
-    URL url(URL(), uri);
+    URL url = convertPlaybinURI(uri);
     if (!urlHasSupportedProtocol(url)) {
         g_set_error(error, GST_URI_ERROR, GST_URI_ERROR_BAD_URI, "Invalid URI '%s'", uri);
         return FALSE;
index 28d8a01..ac336bd 100644 (file)
@@ -1,3 +1,15 @@
+2017-07-06  Charlie Turner  <cturner@igalia.com>
+        [GStreamer] vid.me videos do not play
+        https://bugs.webkit.org/show_bug.cgi?id=172240
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Build httpsoupsrc again for use in adaptive streaming pipelines, and
+        have the existing libsoup build against GNOME to avoid header drift
+        against GStreamer's linked Soup library.
+
+        * gtk/jhbuild.modules:
+
 2017-07-05  Don Olmstead  <don.olmstead@sony.com>
 
         [WTF] Move SoftLinking.h into WTF
index 7d9a113..70f2d6c 100644 (file)
   </autotools>
 
   <autotools id="libsoup"
-             autogenargs="--without-gnome --disable-introspection">
+             autogenargs="--disable-introspection">
     <if condition-set="macos">
       <autogenargs value="--disable-tls-check"/>
     </if>
             hash="sha256:f6d245b6b3d4cb733f81ebb021074c525ece83db0c10e932794b339b8d935eb7"/>
   </autotools>
 
-  <autotools id="gst-plugins-good" autogen-sh="configure" autogenargs="--disable-examples --disable-soup --disable-gtk-doc">
+  <autotools id="gst-plugins-good" autogen-sh="configure" autogenargs="--disable-examples --disable-gtk-doc">
     <if condition-set="macos">
       <autogenargs value="--disable-introspection"/>
     </if>