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

Fix default EXT-X-BYTERANGE offset to start after the previous segment #98

Merged
merged 1 commit into from
Feb 11, 2020
Merged

Fix default EXT-X-BYTERANGE offset to start after the previous segment #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 16 additions & 4 deletions src/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ export default class Parser extends Stream {
discontinuityStarts: [],
segments: []
};
// keep track of the last seen segment's byte range end, as segments are not required
// to provide the offset, in which case it defaults to the next byte after the
// previous segment
let lastByterangeEnd = 0;

// update the manifest with the m3u8 entry from the parse stream
this.parseStream.on('data', function(entry) {
Expand Down Expand Up @@ -89,16 +93,24 @@ export default class Parser extends Stream {
byterange.length = entry.length;

if (!('offset' in entry)) {
this.trigger('info', {
message: 'defaulting offset to zero'
});
entry.offset = 0;
/*
* From the latest spec (as of this writing):
* https://tools.ietf.org/html/draft-pantos-http-live-streaming-23#section-4.3.2.2
*
* Same text since EXT-X-BYTERANGE's introduction in draft 7:
* https://tools.ietf.org/html/draft-pantos-http-live-streaming-07#section-3.3.1)
*
* "If o [offset] is not present, the sub-range begins at the next byte
* following the sub-range of the previous media segment."
*/
entry.offset = lastByterangeEnd;
}
}
if ('offset' in entry) {
currentUri.byterange = byterange;
byterange.offset = entry.offset;
}
lastByterangeEnd = byterange.offset + byterange.length;
},
endlist() {
this.manifest.endList = true;
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/m3u8/byteRange.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
{
"byterange": {
"length": 713084,
"offset": 0
"offset": 1110328
},
"duration": 10,
"timeline": 0,
Expand Down
56 changes: 56 additions & 0 deletions test/m3u8.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1334,6 +1334,62 @@ QUnit.test('Widevine #EXT-X-KEY attributes not attached to manifest if KEYFORMAT
assert.notOk(parser.manifest.contentProtection, 'contentProtection not added');
});

QUnit.test('byterange offset defaults to next byte', function(assert) {
const parser = new Parser();

const manifest = [
'#EXTM3U',
'#EXTINF:5,',
'#EXT-X-BYTERANGE:10@5',
'segment.ts',
'#EXTINF:5,',
'#EXT-X-BYTERANGE:20',
'segment.ts',
'#EXTINF:5,',
'#EXT-X-BYTERANGE:30',
'segment.ts',
'#EXTINF:5,',
'segment2.ts',
'#EXT-X-BYTERANGE:15@100',
'segment.ts',
'#EXT-X-BYTERANGE:17',
'segment.ts',
'#EXT-X-ENDLIST'
].join('\n');

parser.push(manifest);

assert.deepEqual(
parser.manifest.segments[0].byterange,
{ length: 10, offset: 5 },
'first segment has correct byterange'
);
assert.deepEqual(
parser.manifest.segments[1].byterange,
{ length: 20, offset: 15 },
'second segment has correct byterange'
);
assert.deepEqual(
parser.manifest.segments[2].byterange,
{ length: 30, offset: 35 },
'third segment has correct byterange'
);
assert.notOk(parser.manifest.segments[3].byterange, 'fourth segment has no byterange');
assert.deepEqual(
parser.manifest.segments[4].byterange,
{ length: 15, offset: 100 },
'fifth segment has correct byterange'
);
// not tested is a segment with no offset coming after a segment that isn't a sub range,
Copy link
Contributor

Choose a reason for hiding this comment

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

This has me curious on what some valid use-cases may be as the wording in the spec is a bit ambiguous and whether we should add some protections/warnings when we detect that the MUST has been broken.

If o is not present, the sub-range begins at the next byte following the sub-range of the previous Media Segment.

This seems to imply that the subrange begins following the direct previous segment
but then the next sentence

If o is not present, a previous Media Segment MUST appear in the Playlist file and MUST be a sub-range of the same media resource

A previous MediaSeqment, implies that there just needs to be a previous sub-range of the same media resource (segment uri) somewhere in the playlist.

The second sentence makes me think this is a valid playlist
(reduced text for clarity

BYTERANGE:10@5
segment.ts
BYTERANGE:20
segment.ts
DISCO
BYTERANGE:20@100
ad.ts
BYTERANGE:15
ad.ts
DISCO
BYTERANGE:30
segment.ts

The idea with this playlist would be SSAI being injected into an otherwise continuous playlist, and the expectation is that the last segment would have a range of 30@35

We could track the lastByterangeEnd per segment uri to support this use case (if its even a valid one). If we wanted some protections in place, we could default to 0 for when we encounter a byterange without an offset for a specific uri for the first time and trigger a warning, or outright reject like the spec recommends, or do nothing and let things potentially break since its breaking the spec regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting point, and I'm not sure what the spec is implying. The way I read it is that when they use the term "previous" they are referring to the segment immediately preceding, if only because the first sentence says "the previous Media Segment" instead of "a previous Media Segment" like it does in the following. If "previous" could mean any prior segment, then those two statements conflict (unless "the previous Media Segment" is a defined as the segment determined to be the previous one).

So I guess it depends on the definition of "previous." I'm inclined to be strict here, if only because it reduces complexity of the code (no need to manage extra state), and it is easier to relax restrictions in the future than to add on new ones once the spec's definition is clarified.

Copy link
Member

Choose a reason for hiding this comment

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

I think @mjneil is on point here in his interpretation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ask Safari

https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/gear2/byterange_test.m3u8
https://d2zihajmogu5jn.cloudfront.net/bipbop-advanced/gear2/byterange_test_b.m3u8

Copy link
Contributor

Choose a reason for hiding this comment

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

(I still think my interpretation is correct, but I concede)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking. One other thing retaining prior segment info might lead to would be possible corner cases. Dealing with them may be going overboard, but comparing two URIs might involve checking for things like relative paths versus absolute paths, and different forms of the two (for instance, if a 24/7 live stream changes its URI format after an insert/ad break). Again, that might be overboard, but we can always add support for the any prior segment interpretation in the future if the spec's definition becomes clearer.

// as the spec requires that a byterange without an offset must follow a segment that
// is a sub range of the same media resource
assert.deepEqual(
parser.manifest.segments[5].byterange,
{ length: 17, offset: 115 },
'sixth segment has correct byterange'
);
});

QUnit.module('m3u8s');

QUnit.test('parses static manifests as expected', function(assert) {
Expand Down