Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[GStreamer] vid.me videos do not play 
https://bugs.webkit.org/show_bug.cgi?id=172240

Patch by Charlie Turner <cturner@igalia.com> on 2017-07-07
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.

Canonical link: https://commits.webkit.org/191095@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219252 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
@charlie-ht @webkit-commit-queue
charlie-ht authored and webkit-commit-queue committed Jul 7, 2017
1 parent a58b2a9 commit 3214067
Show file tree
Hide file tree
Showing 13 changed files with 227 additions and 15 deletions.
20 changes: 20 additions & 0 deletions LayoutTests/ChangeLog
View file
@@ -1,3 +1,23 @@
2017-07-07 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-06 Michael Catanzaro <mcatanzaro@igalia.com>

Unreviewed GTK and WPE test gardening
Expand Down
3 changes: 3 additions & 0 deletions LayoutTests/http/tests/media/hls/range-request-expected.txt
View file
@@ -0,0 +1,3 @@

EVENT(playing)

28 changes: 28 additions & 0 deletions LayoutTests/http/tests/media/hls/range-request.html
View file
@@ -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>
11 changes: 11 additions & 0 deletions LayoutTests/http/tests/media/resources/hls/range-request-playlist.m3u8
View file
@@ -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"
38 changes: 38 additions & 0 deletions LayoutTests/http/tests/media/resources/hls/range-request-playlists/iframes.m3u8
View file
@@ -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
Binary file added LayoutTests/http/tests/media/resources/hls/range-request-playlists/media.ts
View file
Binary file not shown.
19 changes: 19 additions & 0 deletions LayoutTests/http/tests/media/resources/hls/range-request-playlists/stream.m3u8
View file
@@ -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
64 changes: 64 additions & 0 deletions Source/WebCore/ChangeLog
View file
@@ -1,3 +1,67 @@
2017-07-07 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-07-05 Yusuke Suzuki <utatane.tea@gmail.com>

[WTF] Implement WTF::ThreadGroup
Expand Down
29 changes: 18 additions & 11 deletions Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
View file
Expand Up @@ -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", m_url.string().utf8().data());
g_object_set(m_pipeline.get(), "uri", m_url.string().utf8().data(), nullptr);
}

void MediaPlayerPrivateGStreamer::load(const String& urlString)
{
if (!MediaPlayerPrivateGStreamerBase::initializeGStreamerAndRegisterWebKitElements())
Expand All @@ -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();

Expand All @@ -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.");
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 2 additions & 0 deletions Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
View file
Expand Up @@ -173,6 +173,8 @@ class MediaPlayerPrivateGStreamer : public MediaPlayerPrivateGStreamerBase {
static void uriDecodeBinElementAddedCallback(GstBin*, GstElement*, MediaPlayerPrivateGStreamer*);
static void downloadBufferFileCreatedCallback(MediaPlayerPrivateGStreamer*);

void setPlaybinURL(const URL& urlString);

protected:
void cacheDuration();

Expand Down
12 changes: 10 additions & 2 deletions Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
12 changes: 12 additions & 0 deletions Tools/ChangeLog
View file
@@ -1,3 +1,15 @@
2017-07-07 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-06 Lucas Forschler <lforschler@apple.com>

Write a support script to enable buildbot to upload to S3
Expand Down
4 changes: 2 additions & 2 deletions Tools/gtk/jhbuild.modules
View file
Expand Up @@ -217,7 +217,7 @@
</autotools>

<autotools id="libsoup"
autogenargs="--without-gnome --disable-introspection">
autogenargs="--disable-introspection">
<if condition-set="macos">
<autogenargs value="--disable-tls-check"/>
</if>
Expand Down Expand Up @@ -338,7 +338,7 @@
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>
Expand Down

0 comments on commit 3214067

Please sign in to comment.