Bug 172240 – [GStreamer] vid.me videos do not play

• Component: Media
• Status: RESOLVED FIXED

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

[Charlie Turner]

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.

[Charlie Turner]

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?

[Charlie Turner]

> like GstAppSrc does (which souphttpsrc inherits from

s/GstAppSrc/GstBaseSrc/

[Michael Catanzaro]

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.

[Charlie Turner]

Created attachment 314576 [details]
Patch

[Charlie Turner]

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.

[Michael Catanzaro]

Calvaris, can you review this please?

Charlie, can you create new bugs for the remaining problems?

[Xabier Rodríguez Calvar]

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

[Charlie Turner]

Created attachment 314641 [details]
Patch

[Charlie Turner]

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...

[Charlie Turner]

Michael, yes I will create further bug reports for the other issues identified.

[Xabier Rodríguez Calvar]

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.

[Charlie Turner]

Created attachment 314713 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 314713 [details]
Patch

Clearing flags on attachment: 314713

Committed r219194: <http://trac.webkit.org/changeset/219194>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 174207

[Charlie Turner]

Created attachment 314770 [details]
Patch

[Charlie Turner]

Created attachment 314771 [details]
Patch

[Charlie Turner]

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.

[Michael Catanzaro]

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. ;)

[WebKit Commit Bot]

Comment on attachment 314771 [details]
Patch

Clearing flags on attachment: 314771

Committed r219252: <http://trac.webkit.org/changeset/219252>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

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?

[Charlie Turner]

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.

[Carlos Garcia Campos]

(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.