RESOLVED FIXED 172240
[GStreamer] vid.me videos do not play
https://bugs.webkit.org/show_bug.cgi?id=172240
Summary [GStreamer] vid.me videos do not play
erusan
Reported 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
Attachments
Patch (9.26 KB, patch)
2017-07-04 13:25 PDT, Charlie Turner
no flags
Patch (487.01 KB, patch)
2017-07-05 12:37 PDT, Charlie Turner
no flags
Patch (487.11 KB, patch)
2017-07-06 02:17 PDT, Charlie Turner
no flags
Patch (487.09 KB, patch)
2017-07-06 16:11 PDT, Charlie Turner
no flags
Patch (487.09 KB, patch)
2017-07-06 16:12 PDT, Charlie Turner
no flags
Charlie Turner
Comment 1 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.
Charlie Turner
Comment 2 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?
Charlie Turner
Comment 3 2017-07-03 05:01:51 PDT
> like GstAppSrc does (which souphttpsrc inherits from s/GstAppSrc/GstBaseSrc/
Michael Catanzaro
Comment 4 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.
Charlie Turner
Comment 5 2017-07-04 13:25:24 PDT
Charlie Turner
Comment 6 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.
Michael Catanzaro
Comment 7 2017-07-04 13:58:23 PDT
Calvaris, can you review this please? Charlie, can you create new bugs for the remaining problems?
Xabier Rodríguez Calvar
Comment 8 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
Charlie Turner
Comment 9 2017-07-05 12:37:26 PDT
Charlie Turner
Comment 10 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...
Charlie Turner
Comment 11 2017-07-05 12:42:07 PDT
Michael, yes I will create further bug reports for the other issues identified.
Xabier Rodríguez Calvar
Comment 12 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.
Charlie Turner
Comment 13 2017-07-06 02:17:20 PDT
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2017-07-06 03:12:59 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 16 2017-07-06 10:09:25 PDT
Re-opened since this is blocked by bug 174207
Charlie Turner
Comment 17 2017-07-06 16:11:42 PDT
Charlie Turner
Comment 18 2017-07-06 16:12:18 PDT
Charlie Turner
Comment 19 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.
Michael Catanzaro
Comment 20 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. ;)
WebKit Commit Bot
Comment 21 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>
WebKit Commit Bot
Comment 22 2017-07-07 03:32:44 PDT
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 23 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?
Charlie Turner
Comment 24 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.
Carlos Garcia Campos
Comment 25 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.
Note You need to log in before you can comment on or make changes to this bug.