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-I-FRAME-STREAM-INF #171

Merged
merged 4 commits into from
Jul 6, 2024

Conversation

amtins
Copy link
Contributor

@amtins amtins commented Jul 23, 2023

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

  • 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
  • refactor stream-inf to use the parseResolution function to extract a resolution object

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

fixtures

  • update fixtures to take iFramePlaylists property into account

}

if (event.attributes.RESOLUTION) {
event.attributes.RESOLUTION = parseResolution(event.attributes.RESOLUTION);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich Aug 9, 2023

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 = [];
Copy link
Contributor

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.

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, that's better!

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

harold

Copy link
Contributor

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!

@dzianis-dashkevich dzianis-dashkevich dismissed their stale review August 9, 2023 17:24

Awaiting new changes

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
@amtins amtins force-pushed the feat/i-frame-stream-inf branch 2 times, most recently from 91c70e9 to af6e882 Compare August 12, 2023 13:05
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich left a comment

Choose a reason for hiding this comment

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

LGTM

@mister-ben mister-ben merged commit 990c6ce into videojs:main Jul 6, 2024
2 checks passed
mister-ben added a commit to amtins/m3u8-parser that referenced this pull request Jul 6, 2024
mister-ben added a commit that referenced this pull request Aug 16, 2024
* 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>
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