WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 314576
[details]
Patch
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
Created
attachment 314641
[details]
Patch
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
Created
attachment 314713
[details]
Patch
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
Created
attachment 314770
[details]
Patch
Charlie Turner
Comment 18
2017-07-06 16:12:18 PDT
Created
attachment 314771
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug