Changeset 219194 in webkit


Ignore:
Timestamp:
Jul 6, 2017 3:12:49 AM (7 years ago)
Author:
commit-queue@webkit.org
Message:

[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-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.
Location:
trunk
Files:
7 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r219190 r219194  
     12017-07-06  Charlie Turner  <cturner@igalia.com>
     2
     3        [GStreamer] vid.me videos do not play
     4        https://bugs.webkit.org/show_bug.cgi?id=172240
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        Add a test for single output file HLS playlists that require HTTP
     9        range requests to playback. This failed using the WK http source
     10        for reasons documented in the linked bug.
     11
     12        Generated with mp4hls --segment-duration 3 --output-single-file
     13
     14        * Http/tests/media/hls/range-request-expected.txt: Added.
     15        * http/tests/media/hls/range-request.html: Added.
     16        * http/tests/media/resources/hls/range-request-playlist.m3u8: Added.
     17        * http/tests/media/resources/hls/range-request-playlists/iframes.m3u8: Added.
     18        * http/tests/media/resources/hls/range-request-playlists/media.ts: Added.
     19        * http/tests/media/resources/hls/range-request-playlists/stream.m3u8: Added.
     20
    1212017-07-05  Zalan Bujtas  <zalan@apple.com>
    222

  • trunk/Source/WebCore/ChangeLog

    r219193 r219194  
     12017-07-06  Charlie Turner  <cturner@igalia.com>
     2
     3        [GStreamer] vid.me videos do not play
     4        https://bugs.webkit.org/show_bug.cgi?id=172240
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        In r142251, code to hide the WK HTTP source elements from elsewhere in
     9        the pipeline was removed. This has the nasty side-effect of
     10        auto-plugging the WK HTTP source into things it really should not be
     11        used in, especially the adaptive streaming demuxers. The reasons this
     12        is bad are documented in several places on Bugzilla, see the parent
     13        bug report for more details. The high-level issue is that the WK HTTP
     14        source and its use of WebCore is not thread-safe. Although work has
     15        been recently done to improve this situation, it's still not perfect.
     16
     17        Another issue is the interface hlsdemux expects its HTTP source to
     18        implement, specifically seeking in READY.
     19
     20        This does rely on HTTP context sharing being available in GStreamer,
     21        upstream bug is here:
     22        https://bugzilla.gnome.org/show_bug.cgi?id=761099. The failing case
     23        can be demonstrated with
     24        https://github.com/thiagoss/adaptive-test-server but manual testing on
     25        popular video hosting sites, including vid.me, shows that this doesn't
     26        bite us at the moment, just something else to fix in the future.
     27
     28        There are some QoS issues with the adaptive streaming code in
     29        GStreamer, but it seems much better to offer a below par QoS in lieu
     30        of crashing/livelocking when playing certain streams, and issues can be
     31        raised upstream when they arise.
     32
     33        This patch does take us further away from the future goal of having all
     34        networking operations go through the network process, but in return it
     35        solves some nasty crashes and livelocks that have been irritating
     36        users for some time. With the pressure off on this issue, work can be
     37        planned to consider how to make the WK HTTP source a better citizen
     38        inside the GStreamer pipeline when we migrate the netcode to go
     39        through the network process.
     40
     41        A new test is added to check that the single file HLS playlists
     42        (new in version 4) can be played, which was the primary cause of
     43        this bug report.
     44
     45        Test: http/tests/media/hls/range-request.html
     46
     47        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
     48        (WebCore::MediaPlayerPrivateGStreamer::setPlaybinURL): Perform
     49        some trickery to make sure that we only ever fetch URLs handed to
     50        us by WebCore. Any further URLs discovered inside the pipeline
     51        will not get WKWS auto-plugged, since they'll be plain https?
     52        schemas.
     53        (WebCore::MediaPlayerPrivateGStreamer::load): Refactor to use the
     54        setPlaybinURL helper method.
     55        (WebCore::MediaPlayerPrivateGStreamer::loadNextLocation): Ditto.
     56        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Add
     57        the setPlaybinURL helper method.
     58        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
     59        (webKitWebSrcGetProtocols): Only advertise webkit+https?, this
     60        ensures we won't get auto-plugged by pipeline elements asking for
     61        an element to fetch https? resources (like adaptive demuxers).
     62        (convertPlaybinURI): Undo the trick when another element asks us
     63        for our URI.
     64
    1652017-05-24  Sergio Villar Senin  <svillar@igalia.com>
    266

  • trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp

    r218634 r219194  
    211211}
    212212
     213void MediaPlayerPrivateGStreamer::setPlaybinURL(const URL& url)
     214{
     215    // Clean out everything after file:// url path.
     216    String cleanURLString(url.string());
     217    if (url.isLocalFile())
     218        cleanURLString = cleanURLString.substring(0, url.pathEnd());
     219
     220    m_url = URL(URL(), cleanURLString);
     221
     222    if (m_url.protocolIsInHTTPFamily())
     223        m_url.setProtocol("webkit+" + url.protocol());
     224
     225    GST_INFO("Load %s", cleanURLString.utf8().data());
     226    g_object_set(m_pipeline.get(), "uri", cleanURLString.utf8().data(), nullptr);
     227}
     228
    213229void MediaPlayerPrivateGStreamer::load(const String& urlString)
    214230{
    … …  
    220236        return;
    221237
    222     // Clean out everything after file:// url path.
    223     String cleanURL(urlString);
    224     if (url.isLocalFile())
    225         cleanURL = cleanURL.substring(0, url.pathEnd());
    226 
    227238    if (!m_pipeline)
    228239        createGSTPlayBin();
    … …  
    233244    ASSERT(m_pipeline);
    234245
    235     m_url = URL(URL(), cleanURL);
    236     g_object_set(m_pipeline.get(), "uri", cleanURL.utf8().data(), nullptr);
    237 
    238     GST_INFO("Load %s", cleanURL.utf8().data());
     246    setPlaybinURL(url);
    239247
    240248    if (m_preload == MediaPlayer::None) {
    … …  
    16801688            if (state <= GST_STATE_READY) {
    16811689                // Set the new uri and start playing.
    1682                 g_object_set(m_pipeline.get(), "uri", newUrl.string().utf8().data(), nullptr);
    1683                 m_url = newUrl;
     1690                setPlaybinURL(newUrl);
    16841691                changePipelineState(GST_STATE_PLAYING);
    16851692                return true;

  • trunk/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h

    r218799 r219194  
    174174    static void downloadBufferFileCreatedCallback(MediaPlayerPrivateGStreamer*);
    175175
     176    void setPlaybinURL(const URL& urlString);
     177
    176178protected:
    177179    void cacheDuration();

  • trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp

    r218703 r219194  
    728728const gchar* const* webKitWebSrcGetProtocols(GType)
    729729{
    730     static const char* protocols[] = {"http", "https", "blob", nullptr };
     730    static const char* protocols[] = {"webkit+http", "webkit+https", "blob", nullptr };
    731731    return protocols;
     732}
     733
     734static URL convertPlaybinURI(const char* uriString)
     735{
     736    URL url(URL(), uriString);
     737    ASSERT(url.protocol().substring(0, 7) == "webkit+");
     738    url.setProtocol(url.protocol().substring(7).toString());
     739    return url;
    732740}
    733741
    … …  
    759767        return TRUE;
    760768
    761     URL url(URL(), uri);
     769    URL url = convertPlaybinURI(uri);
    762770    if (!urlHasSupportedProtocol(url)) {
    763771        g_set_error(error, GST_URI_ERROR, GST_URI_ERROR_BAD_URI, "Invalid URI '%s'", uri);

  • trunk/Tools/ChangeLog

    r219191 r219194  
     12017-07-06  Charlie Turner  <cturner@igalia.com>
     2        [GStreamer] vid.me videos do not play
     3        https://bugs.webkit.org/show_bug.cgi?id=172240
     4
     5        Reviewed by Xabier Rodriguez-Calvar.
     6
     7        Build httpsoupsrc again for use in adaptive streaming pipelines, and
     8        have the existing libsoup build against GNOME to avoid header drift
     9        against GStreamer's linked Soup library.
     10
     11        * gtk/jhbuild.modules:
     12
    1132017-07-05  Don Olmstead  <don.olmstead@sony.com>
    214

  • trunk/Tools/gtk/jhbuild.modules

    r218809 r219194  
    218218
    219219  <autotools id="libsoup"
    220              autogenargs="--without-gnome --disable-introspection">
     220             autogenargs="--disable-introspection">
    221221    <if condition-set="macos">
    222222      <autogenargs value="--disable-tls-check"/>
    … …  
    339339  </autotools>
    340340
    341   <autotools id="gst-plugins-good" autogen-sh="configure" autogenargs="--disable-examples --disable-soup --disable-gtk-doc">
     341  <autotools id="gst-plugins-good" autogen-sh="configure" autogenargs="--disable-examples --disable-gtk-doc">
    342342    <if condition-set="macos">
    343343      <autogenargs value="--disable-introspection"/>
Note: See TracChangeset for help on using the changeset viewer.