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 1 commit 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.

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