Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue #1179 with playlist parse #1180

Merged
merged 6 commits into from
Jun 3, 2017
Merged

Fix issue #1179 with playlist parse #1180

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c7ee46
Fix issue #1179 with playlist parse
stevemayhew May 30, 2017
4c5b1e5
Trigger Travis
stevemayhew May 30, 2017
75d024e
WIP: remove broken depend to fix travis build
stevemayhew May 31, 2017
226cfb7
Trigger travis
stevemayhew May 31, 2017
e7d3188
Merge remote-tracking branch 'origin/master' into fix-playlist-parse-…
stevemayhew Jun 1, 2017
8ca06da
Cleanup RegEx init (no commented code). Try longer timeout on selenium
stevemayhew Jun 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/loader/playlist-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. The second capture group (the title) has an explicit match for the trailing space (line termination)
  2. Third capture group (the URI) matches one or more non whitespace
  3. Space removed on the BYTERANGE regex


const LEVEL_PLAYLIST_REGEX_PARTS = [
//'/',
/#EXTINF:(\d*(?:\.\d+)?)(?:,(.*)\s+)?/.source, // duration (#EXTINF:<duration>,<title>), group 1 => duration, group 2 => title
/|(?!#)(\S+)/.source, // segment URI, group 3 => the URI (note newline is not eaten)
/|#EXT-X-BYTERANGE:*(.+)/.source, // next segment's byterange, group 4 => range spec (x@y)
/|#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');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const LEVEL_PLAYLIST_REGEX_SLOW = /(?:(?:#(EXTM3U))|(?:#EXT-X-(PLAYLIST-TYPE):(.+))|(?:#EXT-X-(MEDIA-SEQUENCE): *(\d+))|(?:#EXT-X-(TARGETDURATION): *(\d+))|(?:#EXT-X-(KEY):(.+))|(?:#EXT-X-(START):(.+))|(?:#EXT-X-(ENDLIST))|(?:#EXT-X-(DISCONTINUITY-SEQ)UENCE:(\d+))|(?:#EXT-X-(DIS)CONTINUITY))|(?:#EXT-X-(VERSION):(\d+))|(?:#EXT-X-(MAP):(.+))|(?:(#)(.*):(.*))|(?:(#)(.*))(?:.*)\r?\n?/;

class LevelKey {
Expand Down
28 changes: 26 additions & 2 deletions tests/unit/loader/playlist-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var result = new PlaylistLoader({on : function() { }}).parseLevelPlaylist(level, 'http://proxy-62.dailymotion.com/sec(3ae40f708f79ca9471f52b86da76a3a8)/video/107/282/158282701_mp4_h264_aac_hq.m3u8#cell=core',0);
assert.strictEqual(result.fragments.length, 0);
assert.strictEqual(result.totalduration,0);
Expand Down Expand Up @@ -182,6 +181,31 @@ http://proxy-21.dailymotion.com/sec(2a991e17f08fcd94f95637a6dd718ddd)/video/107/
assert.strictEqual(result.fragments[4].url, 'http://proxy-62.dailymotion.com/sec(3ae40f708f79ca9471f52b86da76a3a8)/frag(5)/video/107/282/158282701_mp4_h264_aac_hq.ts');
});

it('parse level with single char fragment URI', () => {
var level = `#EXTM3U
#EXT-X-ALLOW-CACHE:NO
#EXT-X-TARGETDURATION:2
#EXTINF:2,
0
#EXTINF:2,
1
#EXT-X-ENDLIST`;
var result = new PlaylistLoader({on : function() { }}).parseLevelPlaylist(level, 'http://proxy-62.dailymotion.com/sec(3ae40f708f79ca9471f52b86da76a3a8)/frag(5)/video/107/282/158282701_mp4_h264_aac_hq.m3u8#cell=core',0);
assert.strictEqual(result.totalduration, 4);
assert.strictEqual(result.startSN, 0);
assert.strictEqual(result.targetduration, 2);
assert.strictEqual(result.live, false);
assert.strictEqual(result.fragments.length, 2);
assert.strictEqual(result.fragments[0].cc, 0);
assert.strictEqual(result.fragments[0].duration, 2);
assert.strictEqual(result.fragments[0].sn, 0);
assert.strictEqual(result.fragments[0].relurl, '0');
assert.strictEqual(result.fragments[1].cc, 0);
assert.strictEqual(result.fragments[1].duration, 2);
assert.strictEqual(result.fragments[1].sn, 1);
assert.strictEqual(result.fragments[1].relurl, '1');
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it('parse level with EXTINF line without comma', () => {
var level = `#EXTM3U
#EXT-X-VERSION:3
Expand Down