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

feat: add support for #EXT-X-DEFINE #185

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

mister-ben
Copy link
Contributor

Adds support for #EXT-X-DEFINE.

This requires the parser to have additional information it currently doesn't know: the playlist's URI, and for a media playlist, any variables that have been defined in the main/multivariant playlist. The proposal is to pass an object with the url and defined variables as a constructor argument.

This would need changes to http-streaming to make use of it, to pass those options in.

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Awesome!

Didn't look too closely but looks good on initial glance. I don't remember, do the error triggers stop the rest of the manifest from processing?

The constructor optinally takes an options object with two properties. These are needed when using `#EXT-X-DEFINE` for variable replacement.

```js
var parser = new m3u8Parser.Parser({
Copy link
Member

Choose a reason for hiding this comment

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

this seems reasonable to me. The only thing to consider is whether the options property for definitions should match the value provided by the parser.
So that you could pass the output of the parser from the main playlist directly in to the parser for the media playlist.

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 ideas was that mainDefinitions: main.defintions would be added to the object here, and parseManifest_ would pass that on.

@mister-ben
Copy link
Contributor Author

I don't remember, do the error triggers stop the rest of the manifest from processing?

As is, no, it would be the responsibility of http-streaming or another consumer to listen to that error event and to reject the playlist. Throwing an error and not returning any manifest would be more spec compliant, if we want to do that.

src/parser.js Outdated
// QUERYPARAM attribute, but only one of the three. Otherwise, the
// client MUST fail to parse the Playlist.
this.trigger('error', {
message: 'EXT-X-DEFINE: Invalid attriibutes'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
message: 'EXT-X-DEFINE: Invalid attriibutes'
message: 'EXT-X-DEFINE: Invalid attributes'

src/parser.js Outdated Show resolved Hide resolved
mister-ben and others added 4 commits August 20, 2024 20:32
Co-authored-by: Dzianis Dashkevich <98566601+dzianis-dashkevich@users.noreply.github.com>
@mister-ben mister-ben merged commit ba6e7cb into videojs:main Aug 21, 2024
2 checks passed
@mister-ben mister-ben deleted the feat-ext-x-define branch August 21, 2024 16:10
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.

3 participants