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.

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

Conversation

gesinger
Copy link
Contributor

@gesinger gesinger commented Oct 2, 2019

The default value for EXT-X-BYTERANGE is defined by the spec as the first byte
after the previous segment. Prior to this change, the default set by m3u8-parser
was 0.

The default value for EXT-X-BYTERANGE is defined by the spec as the first byte
after the previous segment. Prior to this change, the default set by m3u8-parser
was 0.
{ 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.

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.

@gkatsev gkatsev self-assigned this Feb 10, 2020
@gesinger gesinger merged commit 08aca73 into videojs:master Feb 11, 2020
miadabdi pushed a commit to miadabdi/m3u8-parser that referenced this pull request Jul 21, 2021
…egment (videojs#98)

The default value for EXT-X-BYTERANGE is defined by the spec as the first byte
after the previous segment. Prior to this change, the default set by m3u8-parser
was 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants