-
Notifications
You must be signed in to change notification settings - Fork 99
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-I-FRAME-STREAM-INF #171
Conversation
} | ||
|
||
if (event.attributes.RESOLUTION) { | ||
event.attributes.RESOLUTION = parseResolution(event.attributes.RESOLUTION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also replace other resolution parsing places with this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzianis-dashkevich thanks for the review!
My first thought was to create a separate PR, as this wasn't the scope of this PR. But if you agree that this PR should include a refactoring commit, I'm all for it. Let me know what you prefer.
I'll do that a little later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought was to create a separate PR, as this wasn't the scope of this PR
This is always a good approach, but I believe it is a pretty small change and will be visually easier to review here.
If you feel differently about it, I am absolutely fine with a separate PR
Thank you!
src/parser.js
Outdated
}, | ||
'i-frame-playlist'() { | ||
if (!this.manifest.iFramePlaylists) { | ||
this.manifest.iFramePlaylists = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a default empty list in the manifest declaration instead of this if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzianis-dashkevich now I remember why I didn't add it to the manifest declaration, because it meant updating a ton of unit tests. If it's OK for you, I'll make the changes this weekend, as I've done enough unit testing for today. 😅
In the meantime, I think it would be best to remove your validation from this PR so that it doesn't get merged by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dzianis-dashkevich I did a force push to include the change and update the commit message. I also added two commits, one updating the test fixtures and the other containing the refactoring. Finally, I rebase main into this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel your pain... Thank you so much for your outstanding contribution!
dfca649
to
5e837a6
Compare
Exposes I-frame playlists through the `iFramePlaylists` property, providing a basis for the creation of trick-play functionality. **parse-stream.js** - add match statement for parsing the `EXT-X-I-FRAME-STREAM-INF` tag - apply type conversions as indicated in the specification for attributes `BANDWIDTH`, `AVERAGE-BANDWIDTH`, `FRAME-RATE` - overwrite the `RESOLUTION` attribute with an object representing the resolution - extract a function to parse the `RESOLUTION` - add test case https://datatracker.ietf.org/doc/html/rfc8216#section-4.3.4.2 **parser.js** - add an array property `iFramePlaylists` to the `manifest` - add each `i-frame playlist` to `iFramePlaylists` - trigger a `warn` event if the `BANDWIDTH` or `URI` attributes are missing, as required by the specification - add test case https://datatracker.ietf.org/doc/html/rfc8216#section-4.3.4.3 - update `master-fmp4.js` to add `iFramePlaylists` - update `README.md` documentation
…resolution object
91c70e9
to
af6e882
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* feat: add support for #EXT-X-I-FRAMES-ONLY Handles I-frames-only `segments`, providing a basis for the creation of trick-play functionality. **parse-stream.js** - add match statement for parsing the `EXT-X-I-FRAMES-ONLY` tag - add test case **parser.js** - add a property `iFramesOnly` to the `manifest` - add a function to validate the minimum version required - trigger a `warn` event if the minimum version required is not supported or undefined, as required by the specification - add test case https://datatracker.ietf.org/doc/html/rfc8216#section-4.3.3.6 - update `README.md` documentation * Update src/parse-stream.js * Update test for #171 changes --------- Co-authored-by: mister-ben <1676039+mister-ben@users.noreply.github.com>
Description
Exposes I-frame playlists through the
iFramePlaylists
property, providing a basis for the creation of trick-play functionality.Specific Changes proposed
parse-stream.js
EXT-X-I-FRAME-STREAM-INF
tagBANDWIDTH
,AVERAGE-BANDWIDTH
,FRAME-RATE
RESOLUTION
attribute with an object representing the resolutionRESOLUTION
stream-inf
to use theparseResolution
function to extract aresolution
objecthttps://datatracker.ietf.org/doc/html/rfc8216#section-4.3.4.2
parser.js
iFramePlaylists
to themanifest
i-frame playlist
toiFramePlaylists
warn
event if theBANDWIDTH
orURI
attributes are missing, as required by the specificationhttps://datatracker.ietf.org/doc/html/rfc8216#section-4.3.4.3
master-fmp4.js
to addiFramePlaylists
README.md
documentationfixtures