Bug 172240 - [GStreamer] vid.me videos do not play
Summary: [GStreamer] vid.me videos do not play
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on: 174207
Blocks:
  Show dependency treegraph
 
Reported: 2017-05-17 13:32 PDT by erusan
Modified: 2017-07-10 06:31 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.26 KB, patch)
2017-07-04 13:25 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (487.01 KB, patch)
2017-07-05 12:37 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (487.11 KB, patch)
2017-07-06 02:17 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (487.09 KB, patch)
2017-07-06 16:11 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch (487.09 KB, patch)
2017-07-06 16:12 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description erusan 2017-05-17 13:32:07 PDT
Virtually no video on vid.me plays.

On Debian unstable, tested with Epiphany 3.22.x and 3.24.1,
libwebkit2gtk 2.16.2 and earlier

gstreamer1.0-plugins-(bad/base/good/ugly) installed.

Flash not installed.

Essentially ever other video website works with HTML5 video and without Flash. One or two videos on vid.me have played.

A brief loading icon shows in the player, and video controls and thumbnails are shown. However, videos themselves do not load.

One video that does load is the narwhal video on this page: https://vid.me/about
Comment 1 Charlie Turner 2017-06-21 08:46:11 PDT
Thanks for reporting this. I will take a look.

Most videos on vid.me advertise HLS streaming, which we attempt to play but fail. The video on the about page advertises a DASH stream with MPEG-4 fallbacks which we can play successfully.
Comment 2 Charlie Turner 2017-07-03 03:04:26 PDT
So, this is actually a fairly hairy unsolved issue in the HLS playback.

The first issue you get with vid.me is that it serves HLS content with byte ranges. Any such service will fail to load, and internally gst will sit in an infinite loop within hlsdemux trying to send seek events to our HTTP source element... That fails because HLS with byte ranges is a special case inside gstreamer, and our HTTP source doesn't support seek requests in it's READY mode, which is what hlsdemux assumes is true of its httpsrc. The problem with this behaviour from WK's perspective is that we can't detect the error and fallback to a different source, we just sit there busily doing nothing ;)

WebKitWebSrc (WKWS) is a GstBin, which doesn't handle seek in ready for us, like GstAppSrc does (which souphttpsrc inherits from). See also https://bugzilla.gnome.org/show_bug.cgi?id=696952

I tried adding a seek in ready feature to WKWS, and after some other dubious changes to get WKWS not to deadlock or crash in this configuration, I can get vid.me content to play, but the video and audio is ridiculously choppy, and it seems to be when ever hlsdemux switches bitrates. This could be futher investigated, but it started feeling like WKWS isn't suitable for hlsdemux's use case. There was some acknowledgement of that here: https://bugs.webkit.org/show_bug.cgi?id=149672

After looking elsewhere at this point I stumbled on old work to do with HTTP context sharing, and use the WKWS just to grab the main manifest file, and let souphttpsrc be used to fetch the fragments internally, sharing the context of WKWS for cookies/authentication. That sounds like a better idea to me. It's mentioned in https://bugs.webkit.org/show_bug.cgi?id=144040 and an attempt to do it was in https://bugs.webkit.org/show_bug.cgi?id=108088. #108088 is stalled on changes in upstream gstreamer https://bugzilla.gnome.org/show_bug.cgi?id=761099, which has been dormant for quite some time. It's not clear from these bugs what people think about this idea and whether it should be pursued.

So in summary,

1) HLS works in some cases but not in others, I guess it's debatable whether we should even advertise HLS mime support inside WK. That could be the quickest fix, just don't advertise HLS support and fallback to other sources if they exist.
2) Could investigate more this seek in ready support, but it's kinda gross to be duplicating functionality that already exists in gstreamer, and invetiably drifting from it as upstream changes, but we sort of do this anyway in WKWS.
3) Could push HTTP context sharing further in gstreamer and use soup/neon internally instead.

3) sounds best to me as a way forward, thoughts?
Comment 3 Charlie Turner 2017-07-03 05:01:51 PDT
> like GstAppSrc does (which souphttpsrc inherits from

s/GstAppSrc/GstBaseSrc/
Comment 4 Michael Catanzaro 2017-07-03 07:12:39 PDT
I'm inclined to say we should not advertise HLS support if it is not working, but unfortunately HLS is something we've "supported" (badly) for years... we can't just take it away at this point. So this is just going to have to remain broken until it can be fixed properly.
Comment 5 Charlie Turner 2017-07-04 13:25:24 PDT
Created attachment 314576 [details]
Patch
Comment 6 Charlie Turner 2017-07-04 13:31:48 PDT
Just as some extra commentary to that already in the ChangeLogs,

- This does move us further away from having netcode being routed through the network process. It seemed to me a better tradeoff to having our advertised functionality working okay than to be closer to a future goal. I know that's high debatable, but worth throwing out there.

- The WKWS isn't thread safe, something that's heavily relied on inside the pipeline. It's definitely got a lot better, but still the threading model inside WKWS is IMHO very confusing and brittle. It could use some rework, but while doing that, it would be nice to test in development and not hurt users in unexpected ways and instead to rely on the souphttpsrc for adaptive streaming.

- Sites that protect streams using cookies on master playlists won't work at the moment, but I haven't found a popular one that does this. You can produce the failure using https://github.com/thiagoss/adaptive-test-server. Once we get https://bugzilla.gnome.org/show_bug.cgi?id=761099 in, this can be fixed properly. But in the meantime I think it's better to have this known issue than unknown threading problems.

- GStreamer's HLS implementation needs some work to get a QoS similar to the Apple platforms, but again, known knowns.
Comment 7 Michael Catanzaro 2017-07-04 13:58:23 PDT
Calvaris, can you review this please?

Charlie, can you create new bugs for the remaining problems?
Comment 8 Xabier Rodríguez Calvar 2017-07-05 00:39:11 PDT
Comment on attachment 314576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314576&action=review

> Source/WebCore/ChangeLog:9
> +        the pipeline was removed. This has the nasty side-affect of

side-effect

> Source/WebCore/ChangeLog:11
> +        used in, especially the adapative streaming demuxers. The reasons this

adaptative

> Source/WebCore/ChangeLog:14
> +        source and it's use of WebCore is not thread-safe. Although work has

its

> Source/WebCore/ChangeLog:30
> +        of crashing/livelocking when playin certain streams, and issues can be

playing

> Source/WebCore/ChangeLog:31
> +        raised upstream when they arrise.

arise?

> Source/WebCore/ChangeLog:41
> +        Covered by existing multimedia tests.

Which test covers this? We need to have at least one test that was not passing before and it is passing now since this is not "just a rework".

> Source/WebCore/ChangeLog:51
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +        (WebCore::MediaPlayerPrivateGStreamer::setPlaybinURL):
> +        (WebCore::MediaPlayerPrivateGStreamer::load):
> +        (WebCore::MediaPlayerPrivateGStreamer::loadNextLocation):
> +        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
> +        (webKitWebSrcGetProtocols):
> +        (convertPlaybinURI):
> +        (webKitWebSrcSetUri):

It would be good if you could explain a bit the code here. The changelog explains the motivation, etc but nothing specific about what it code is doing.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:734
> +static URL convertPlaybinURI(const gchar* uriString)

char*

> Tools/ChangeLog:9
> +        against the gstreamer linked soup.

GStreamer and Soup
Comment 9 Charlie Turner 2017-07-05 12:37:26 PDT
Created attachment 314641 [details]
Patch
Comment 10 Charlie Turner 2017-07-05 12:38:54 PDT
Thanks for review, sorry for the sloppy English...

Reviewer: The test case is a bit janky. Ideally we'd use the progress/stalled events to detect failure, but there's a rats nest of bugs generated those events already, so I had to make do with the playing event. Let me know if there's a better way...
Comment 11 Charlie Turner 2017-07-05 12:42:07 PDT
Michael, yes I will create further bug reports for the other issues identified.
Comment 12 Xabier Rodríguez Calvar 2017-07-06 01:43:01 PDT
Comment on attachment 314641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=314641&action=review

> Tools/ChangeLog:7
> +        Build httpsoupsrc again for use in adapative streaming pipelines, and

"adapative" again :) We should ask the Queen to get it added to the dictionary already :)

> LayoutTests/http/tests/media/hls/range-request.html:23
> +            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>

You need to check this indentation.
Comment 13 Charlie Turner 2017-07-06 02:17:20 PDT
Created attachment 314713 [details]
Patch
Comment 14 WebKit Commit Bot 2017-07-06 03:12:55 PDT
Comment on attachment 314713 [details]
Patch

Clearing flags on attachment: 314713

Committed r219194: <http://trac.webkit.org/changeset/219194>
Comment 15 WebKit Commit Bot 2017-07-06 03:12:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Commit Bot 2017-07-06 10:09:25 PDT
Re-opened since this is blocked by bug 174207
Comment 17 Charlie Turner 2017-07-06 16:11:42 PDT
Created attachment 314770 [details]
Patch
Comment 18 Charlie Turner 2017-07-06 16:12:18 PDT
Created attachment 314771 [details]
Patch
Comment 19 Charlie Turner 2017-07-06 16:20:40 PDT
The difference here is that now the pipeline is actually getting the converted URL sent to it... 

g_object_set(m_pipeline.get(), "uri", m_url.string().utf8().data(), nullptr);

Rather than

g_object_set(m_pipeline.get(), "uri", cleanURL.utf8().data(), nullptr);

Managed to miss the failing tests in http/ because I mistakenly identified them as flaky. Carefully checked this doesn't introduce regressions now.
Comment 20 Michael Catanzaro 2017-07-06 18:14:54 PDT
Since Calvaris already reviewed it and you've only made a minor change, you don't have to request review again. Instead, you can just copy his name in the ChangeLog over top the NOBODY (OOPS!). This would actually be done for you if you grabbed your patch using 'webkit-patch apply-from-bug 172240'.

The advantage of that is I could cq+ it now while leaving Calvaris as the reviewer. Since I can't do that, you will have to wait until he looks at it again tomorrow. ;)
Comment 21 WebKit Commit Bot 2017-07-07 03:32:42 PDT
Comment on attachment 314771 [details]
Patch

Clearing flags on attachment: 314771

Committed r219252: <http://trac.webkit.org/changeset/219252>
Comment 22 WebKit Commit Bot 2017-07-07 03:32:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Carlos Garcia Campos 2017-07-10 05:41:07 PDT
It's great we can do this now, thanks! I guess it also means we can remove all the ResourceHandleStreamingClient implementation, since it was only used by HLS, right?
Comment 24 Charlie Turner 2017-07-10 06:09:07 PDT
That and all the "if I'm not in the main thread, do this" code paths IIUC. I wanted to keep this commit fairly small and remove code in a different one just while the dust settles.

Unless you disagree with that approach, I have a list of bug reports I need to create and that is one of them, to remove dead code.
Comment 25 Carlos Garcia Campos 2017-07-10 06:31:01 PDT
(In reply to Charlie Turner from comment #24)
> That and all the "if I'm not in the main thread, do this" code paths IIUC. I
> wanted to keep this commit fairly small and remove code in a different one
> just while the dust settles.
> 
> Unless you disagree with that approach, I have a list of bug reports I need
> to create and that is one of them, to remove dead code.

Sure, I agree, just wanted to make sure we were aware of the dead code now. I'm not sure if it's possible to create src elements from a different thread, I think web audio implementation did that too, but I'm not sure at all.