Hls.js: MPEG-TS fragments with spaces in the filename are not parsed correctly

[kennymalac]

Environment

• [x] The stream has correct Access-Control-Allow-Origin headers (CORS)
• [ ] There are no network errors such as 404s in the browser console when trying to play the stream
• [x] The issue observed is not already reported by searching on Github under https://github.com/video-dev/hls.js/issues
• [x] The issue occurs in the latest reference client on http://video-dev.github.io/hls.js/demo and not just on my page
• Link to playable M3U8 file: https://s3-us-west-2.amazonaws.com/tru-dev/transcoded/4/course/10/lecture/12/video/2017-11-04+13-24-28/hls_1200k_2017-11-04+13-24-28_171121082127_v4.m3u8
• Hls.js version: latest from CDN, should be 0.8.7

• Browser name/version: Firefox 57
• OS name/version: Ubuntu 17.04

Steps to reproduce

have a m3u8 fragment with a space in the filename, demo shown below:
http://video-dev.github.io/hls.js/demo/?src=GETXHRhttps%3A%2F%2Fs3-us-west-2.amazonaws.com%2Ftru-dev%2Ftranscoded%2F4%2Fcourse%2F10%2Flecture%2F12%2Fvideo%2F2017-11-04%252013-24-28%2Fhls_2017-11-04%252013-24-28_171121082127.m3u8%20&enableStreaming=true&autoRecoverError=true&enableWorker=true&dumpfMP4=false&levelCapping=-1&defaultAudioCodec=undefined

Expected behavior

Should load the fragment no problem (flash hls plays it, for example)

Actual behavior

The file extension (.ts) is omitted, network requests are made for "2017-11-04%2013-24-28/hls_1200k_2017-11-04" without the proper extension

Console output

logger.js:37:6
[log] > loadSource:GETXHRhttps://s3-us-west-2.amazonaws.com/tru-dev/transcoded/4/course/10/lecture/12/video/2017-11-04%2013-24-28/hls_2017-11-04%2013-24-28_171121082127.m3u8
logger.js:37:6
[error] > internal error happened while processing hlsManifestLoading:A network error occurred.
logger.js:37:6
[log] > trigger BUFFER_RESET
logger.js:37:6
[log] > set autoLevelCapping:-1
logger.js:37:6
[log] > attachMedia
logger.js:37:6
[log] > media source opened
logger.js:37:6
[warn] > timeout while loading GETXHRhttps://s3-us-west-2.amazonaws.com/tru-dev/transcoded/4/course/10/lecture/12/video/2017-11-04%2013-24-28/hls_2017-11-04%2013-24-28_171121082127.m3u8
logger.js:37:6
Object { type: "networkError", details: "manifestLoadTimeOut", fatal: true, url: undefined, loader: {…}, context: {…}, networkDetails: null }
 ^ ^ ^ […]0: "hlsError"1: {…}details: "fragLoadError"fatal: truefrag: Object { _url: "https://s3-us-west-2.amazonaws.com/tru-dev/transcoded/4/course/10/lecture/12/video/2017-11-04%2013-24-28/hls_1200k_2017-11-04", duration: 1.7, rawByteRange: "36096@0", … }networkDetails: XMLHttpRequest { readyState: 4, timeout: 0, withCredentials: false, … }response: Object { code: 403, text: "Forbidden" }type: "networkError"__proto__: Object { … }length: 2__proto__: Array [] mediaelement-and-player.js:6615:6
// Here S3 complains about not having bucket permissions with the 403 Error ^
demo:528:9
fatal error :manifestLoadTimeOut
demo:589:11
timeout while loading manifest,network error ...
demo:602:11

[benjamingr]

We've ran into this a few times with clients - old versions of hls.js actually did work on manifests with spaces.

This is according to the spec though - and hls.js is correct to reject the manifest.

[benjamingr]

Namely an HLS manifest contains URIs:

Each Media Segment is specified by a series of Media Segment tags followed by a URI.

And a URI cannot contain spaces inside the URI. (Check out the segment called Collected ABNF for URI)

[kennymalac]

Oh okay. There's no problem with that then other than that Elastic Transcoder potentially generates improper m3u8s! The likelihood of this getting fixed by Amazon in the near future is next to nil though, so I've replaced spaces in my video files with underscores. Still, this is quite a nasty error response. Rather than failing to GET the file, the parser should try to get a "proper" URI, and if the potential-URI is improper, either that should be an error, or it should try out the improper URI. Neither option is preferable, because with the second option it wouldn't fit the spec, and on the first, many developers might still get confused; especially those with user-facing upload services, where the filename is uncontrollable (though, of course, if you had the preconception you had to get rid of spaces because you know Elastic Transcoder didn't replace them, and the HLS parser would fail, you would).

Those are just my comments on the issue, you can mark this as "wontfix"/close the issue if it's not worth the effort. :smile:

[mangui]

u should be able to make it work by adding a encodeURI() here
https://github.com/video-dev/hls.js/blob/master/src/loader/playlist-loader.js#L238

[benjamingr]

@kennymalac to clarify, I wasn't arguing for hls.js to not handle it. I was just trying to help by explaining why it is happening in the first place.

[kennymalac]

@benjamingr no worries, thank you!

@mangui I will try to open a pull request when I have the time but I noticed in https://github.com/video-dev/hls.js/blob/master/tests/unit/loader/playlist-loader.js there is not a unit test that tests the resolve() method specifically and I was wondering if that's something that should be done, and where in the unit test I could put that. Just a couple pointers would be nice.

[mangui]

@kennymalac feel free to add some

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dtniland]

We're running into this issue as well. Ironically, Safari loads the ts fragment urls just fine, so arguing that it's not it the spec is a little weak. Rather, it looks like the regex you are using to parse the levels manifests uses spaces as separators -- something that seems unnecessary as manifests are meant to be parsed per line. By parsing it in this way you've created an issue where there should be none.
Regex is almost impossible to understand let alone debug and modify. Was there a speed improvement that justified making that parsing code so much more brittle?

[tchakabam]

@dtniland Parsing M3u8 is more complex than one might think. A lot of things are allowed/possible. The regex covers those pretty well.

Admittedly, sure a lot of stuff isn't ideal in the current codebase, and any constructive input is welcome.

But I don't think the regex approach in general is to blame. it's just needs to be changed in order to parse line-by-line.

Moreover one could very well split into lines in the first place and then pass those to the regex.

We'll be happy to look at and help with constructive approaches to improve the code! :)

However doing the M3u8 lines parsing without a regex is actually also resulting in quite complex code in my experience.

[tchakabam]

@dtniland Eventually it could be fixed quite easily by URI-encoding/escaping the M3u8 before parsing.

[tchakabam]

Here is an M3u8 parser I wrote once in plain Java: https://gist.github.com/tchakabam/81be43a83a7431551dc447c55e0a91f4

It might be less condensed than the Hls.js code, but you will see that it doesn't manage easily without regex either, and also that the format is eventually complex when one looks at the cases to cover. And it lacks any kind of data-semantics like XML or JSON and has a lot of "special" stuff to cover ... :)

[dtniland]

Ok, this wasn't as complicated to fix as I thought it would be. I guess I have a visceral reaction to regex. (I'm still not clear what the utility is for using it, but that's another issue) I made a PR with a fix if you want to incorporate it: https://github.com/video-dev/hls.js/pull/1899

[tchakabam]

@dtniland I feel you about the regexes. In fact the thing is, if you want to write parser code without regex to cover all the cases of M3u8 lines, you will see that it is very complex - about as complex as understanding that regex :) maybe you know about https://regex101.com/, it's pretty useful. once you re into it, you start liking regex because it's pretty powerful and can spare you writing a lot of dumb parser code, and it covers many cases at once.