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

Support for playlist version check #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Wkkkkk
Copy link

@Wkkkkk Wkkkkk commented Feb 10, 2025

This first commit is a trial implementation for version checking and could be used for discussions.

@Wkkkkk Wkkkkk requested a review from tobbee February 10, 2025 11:02
@Wkkkkk Wkkkkk self-assigned this Feb 10, 2025
Copy link
Collaborator

@tobbee tobbee left a comment

Choose a reason for hiding this comment

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

Let's have a discussion on where we aim.

I think I would rather like a method like "CalculateMinimalVersion()" on the MasterPlaylist and MediaPlaylist. That method could then be added to the Playlist interface.

We can then just run through the steps for minimal version checking for the two types of playlists, and every time it increases we save a reason why it increased. At the end we should have the reason for version that is returned.

In the reader, we should not have any automatic check for version, but let the user check at the end using the method if he/she finds it important.

Of course, we could do something with "strict" parsing, but that mode is not generally implemented in a good way, so I don't think it is worth the effort.

Before encoding, it may be useful to be set the right version, so that is a more relevant case, in my view. Still, I think it should be a manual test, and not automatic.

Regarding the current code in the PR, I think it is more complex than it needs to be. In my view, we just need some loops to check the conditions one by one and reporting a version and a reason.

@Wkkkkk
Copy link
Author

Wkkkkk commented Feb 13, 2025

Hi @tobbee, I tried to implement the checks as a function for Playlist, but it seems rather strange since many of these rules are more related to the decoding state.

Take this rule for example.

A Media Playlist MUST indicate an EXT-X-VERSION of 3 or higher 
if it contains: Floating-point EXTINF duration values.

It would be more natural if this check is done in decodeLineOfMediaPlaylist where we parsed the string into floats.

Following the same reason I added two more rules and would ask for your review again.

@Wkkkkk Wkkkkk requested a review from tobbee February 13, 2025 08:25
@tobbee
Copy link
Collaborator

tobbee commented Feb 13, 2025

In my view, the main usefulness is till to be able to set a proper version on a playlist that you have created depending on what features have been added to the segments.

Verification of the version compliance when parsing a playlist is another thing, and less interesting because you typically try to use and parse whatever you get.

I don't consider the tests for version 2 important, since the library is said to only support version 3, but if one wants to make a (imprerfect) test of the whether the segment durations are integral, one can check if they have any decimals. With a floating point decimal configuration of "-1", they will even be output as integers.

Since we have different views on how this should be done, I'll try to make a quick implementation myself and see if it works out or if I stumble on the same issues as you.

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.

2 participants