Fix issue #1179 with playlist parse #1180

stevemayhew · 2017-05-30T22:34:50Z

Description of the Changes

This fixes issue #1179.

For encrypted playlist that use the SN as the initialization vector the content will not play at all. This is because the playlist regex skips the first N segments and sets the SN to 0 for the Nth segment.
For unencrypted content it simply skips segments.

The problem was with the regex, it does not deal with segment URL that is one character or less.

See checkin/pull comments for why.

CheckLists

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
Travis tests are passing (or test results are not worse than on master branch :))
API or design changes are documented in API.md

stevemayhew · 2017-05-30T22:42:57Z

src/loader/playlist-loader.js

@@ -12,7 +12,17 @@ import {logger} from '../utils/logger';
 // https://regex101.com is your friend
 const MASTER_PLAYLIST_REGEX = /#EXT-X-STREAM-INF:([^\n\r]*)[\r\n]+([^\r\n]+)/g;
 const MASTER_PLAYLIST_MEDIA_REGEX = /#EXT-X-MEDIA:(.*)/g;
-const LEVEL_PLAYLIST_REGEX_FAST = /#EXTINF:(\d*(?:\.\d+)?)(?:,(.*))?|(?!#)(\S.+)|#EXT-X-BYTERANGE: *(.+)|#EXT-X-PROGRAM-DATE-TIME:(.+)|#.*/g;
+//const LEVEL_PLAYLIST_REGEX_FAST = /#EXTINF:(\d*(?:\.\d+)?)(?:,(.*)\s+)?|(?!#)(\S+)|#EXT-X-BYTERANGE:*(.+)|#EXT-X-PROGRAM-DATE-TIME:(.+)|#.*/g;


Left the original format for the RegEx commented out so the exact changes are easy to see

The second capture group (the title) has an explicit match for the trailing space (line termination)

Third capture group (the URI) matches one or more non whitespace

Space removed on the BYTERANGE regex

stevemayhew · 2017-05-30T22:45:16Z

src/loader/playlist-loader.js

+  /|#EXT-X-PROGRAM-DATE-TIME:(.+)/.source,       // next segment's program date/time group 5 => the datetime spec
+  /|#.*/.source                                  // All other non-segment oriented tags will match with all groups empty
+];
+const LEVEL_PLAYLIST_REGEX_FAST = new RegExp(LEVEL_PLAYLIST_REGEX_PARTS.join(''), 'g');


Split the regex up by alternatives, IMHO makes it easier to read and maintain. You can load the code and print LEVEL_PLAYLIST_REGEX_PARTS.join('') if one needs to paste the whole expression into something like regex101.

Otherwise the code is exactly the same as the commented out line 15 above.

I agree that the regexp is complex and splitting it helps to read and maintain.
However in this case you are creating real Regexp (that are never used) , then converting them to string using Regexp.prototype.source, whereas you could directly join strings

stevemayhew · 2017-05-30T22:46:49Z

tests/unit/loader/playlist-loader.js

@@ -139,8 +139,7 @@ http://proxy-21.dailymotion.com/sec(2a991e17f08fcd94f95637a6dd718ddd)/video/107/
    var level = `#EXTM3U
 #EXT-X-VERSION:3
 #EXT-X-PLAYLIST-TYPE:VOD
-#EXT-X-TARGETDURATION:14
-#EXTINF:11.360,`;
+#EXT-X-TARGETDURATION:14`;


The #EXTINF without the required segment (or at least any space character or other tag) following it will not parse with the updated regex. If there is some real code that generates this invalid playlist then we can work on the regex to deal with this explicitly.

stevemayhew · 2017-05-30T22:47:25Z

tests/unit/loader/playlist-loader.js

+     assert.strictEqual(result.fragments[1].sn, 1);
+     assert.strictEqual(result.fragments[1].relurl, '1');
+  });
+


This is the new test case, that fails with the old code.

stevemayhew · 2017-05-31T00:12:23Z

This dependency is causing travisci to fail... I'll look at fixing it tomorrow.


├─┬ live-reload@1.1.0
...
│ ├─┬ reconnect@0.1.5
│ │ ├── backoff@1.0.0
│ │ ├── h@0.1.0
│ │ └─┬ shoe@0.0.15
│ │   ├─┬ sockjs@0.3.7
│ │   │ ├── faye-websocket@0.4.4
│ │   │ └── node-uuid@1.3.3 deduped
│ │   └── sockjs-client@0.0.0-unreleasable
│ ├─┬ shoe@0.0.5
│ │ ├── sockjs-client@0.0.0-unreleasable (git://github.com/substack/sockjs-client.git#40d48d06b4dba884416bf88a051f76ca3c8ffcae)

The sockjs-client is the issue. Suggest removing watchify and live-reload for travis builds.

mangui

thanks @stevemayhew !
you might optimize the regexp array using plain string

mangui · 2017-05-31T03:32:36Z

src/loader/playlist-loader.js

+  /|#EXT-X-PROGRAM-DATE-TIME:(.+)/.source,       // next segment's program date/time group 5 => the datetime spec
+  /|#.*/.source                                  // All other non-segment oriented tags will match with all groups empty
+];
+const LEVEL_PLAYLIST_REGEX_FAST = new RegExp(LEVEL_PLAYLIST_REGEX_PARTS.join(''), 'g');


I agree that the regexp is complex and splitting it helps to read and maintain.
However in this case you are creating real Regexp (that are never used) , then converting them to string using Regexp.prototype.source, whereas you could directly join strings

mangui · 2017-06-01T05:10:18Z

277fb1c should address the build issue. you should be able to revert your Travis related commits

stevemayhew · 2017-06-01T21:08:11Z

Thanks.. The const LEVEL_PLAYLIST_REGEX_FAST = new RegExp(LEVEL_PLAYLIST_REGEX_PARTS.join('') is the one actually being used. The old one is just a comment so you can see the diffs.

Your call, I think it makes it clearer to split it up into multiple // literal regex then join the string source in the end.

If you do the regex as a string and make it multi-line it's a real mess cause you need to escape all the \'s

So for example something nice like this:

  /#EXTINF:(\d*(?:\.\d+)?)(?:,(.*))?|/

Has to be:

    '#EXTINF:(\\d*(?:\\.\\d+)?)(?:,(.*))?|'

And you can't paste that into RegEx101 ;-)

Again, your call. If you like it split up I'll delete the commented out code in the final commit

stevemayhew · 2017-06-01T21:09:38Z

PS thanks for fixing live-reload.. I'll merge again with mainline and update the pull

…issue Resolve conflict by taking remote package.json (Hopefully fixing the Shoe issue with Travis)

mangui · 2017-06-03T05:32:15Z

looks good, thanks !
merging

Fix issue video-dev#1179 with playlist parse

6c7ee46

stevemayhew commented May 30, 2017

View reviewed changes

Trigger Travis

4c5b1e5

mangui reviewed May 31, 2017

View reviewed changes

stevemayhew added 2 commits May 31, 2017 12:09

WIP: remove broken depend to fix travis build

75d024e

Trigger travis

226cfb7

stevemayhew added 2 commits June 1, 2017 14:15

Merge remote-tracking branch 'origin/master' into fix-playlist-parse-…

e7d3188

…issue Resolve conflict by taking remote package.json (Hopefully fixing the Shoe issue with Travis)

Cleanup RegEx init (no commented code). Try longer timeout on selenium

8ca06da

mangui merged commit 3a3dd20 into video-dev:master Jun 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue #1179 with playlist parse #1180

Fix issue #1179 with playlist parse #1180

stevemayhew commented May 30, 2017 •

edited

Loading

stevemayhew May 30, 2017

stevemayhew May 30, 2017

mangui May 31, 2017

stevemayhew May 30, 2017

stevemayhew May 30, 2017

stevemayhew commented May 31, 2017

mangui left a comment

mangui May 31, 2017

mangui commented Jun 1, 2017

stevemayhew commented Jun 1, 2017 •

edited

Loading

stevemayhew commented Jun 1, 2017

mangui commented Jun 3, 2017

Fix issue #1179 with playlist parse #1180

Fix issue #1179 with playlist parse #1180

Conversation

stevemayhew commented May 30, 2017 • edited Loading

Description of the Changes

CheckLists

stevemayhew May 30, 2017

Choose a reason for hiding this comment

stevemayhew May 30, 2017

Choose a reason for hiding this comment

mangui May 31, 2017

Choose a reason for hiding this comment

stevemayhew May 30, 2017

Choose a reason for hiding this comment

stevemayhew May 30, 2017

Choose a reason for hiding this comment

stevemayhew commented May 31, 2017

mangui left a comment

Choose a reason for hiding this comment

mangui May 31, 2017

Choose a reason for hiding this comment

mangui commented Jun 1, 2017

stevemayhew commented Jun 1, 2017 • edited Loading

stevemayhew commented Jun 1, 2017

mangui commented Jun 3, 2017

stevemayhew commented May 30, 2017 •

edited

Loading

stevemayhew commented Jun 1, 2017 •

edited

Loading