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 explicit 'Duration=' parameter to EXT-X-CUE-OUT #362

Merged
merged 3 commits into from
May 9, 2024

Conversation

Equipo45
Copy link
Contributor

@Equipo45 Equipo45 commented May 9, 2024

Description

I have been investigating the problem with DURATION parameters.

Firstly, as you already know it isn't compulsory to establish the parameter as you can see in an example where is use and other which not. The use of the parameter will depend on some key points:

  1. Is there another parameter indicating two or more attributes about the ad break EX: #EXT-X-CUE-OUT:DURATION=195.000,BREAKID=103038
  2. Does the parser or model used for generating the manifest support it?

I see the change as an improvement for the following reasons:

  1. It adds clarity to the parameter that is being introduced by adding it explicitly.
  2. Is currently used as a standard by reference companies such as Google.

Regarding all, thank you for making the package and contributing to the open-source community.

A test has been changed for checking the new format

Copy link
Contributor

@leandromoreira leandromoreira left a comment

Choose a reason for hiding this comment

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

thanks, good work, I believe we must add the parser changes to this pr as well.

m3u8/model.py Outdated
@@ -629,7 +629,7 @@ def dumps(self, last_segment, timespec="milliseconds"):

output.append(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should preserve what the what we receive as input.

Meaning, we should expand the parser to include something like cue_out_duration_property to true.

        elif line.startswith(protocol.ext_x_cue_out):
            _parse_cueout(line, state)
            state["cue_out_start"] = True
            state["cue_out"] = True
            if line.contains(somethingsomething):
                   state["cue_out_explicitly_duration"] = True

And then here, we would need to check to see if we'd need to add the Duration or not. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise we'll transform manifests without Duration to have one.

Copy link
Contributor Author

@Equipo45 Equipo45 May 9, 2024

Choose a reason for hiding this comment

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

Yeah, that's true. I changed it and adapted for parsing it only when DURATION is available.
Also, a new test has been created and some code has been refactored regarding the new state.

Copy link
Contributor

@leandromoreira leandromoreira left a comment

Choose a reason for hiding this comment

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

thanks for the great work

Copy link
Member

@mauricioabreu mauricioabreu left a comment

Choose a reason for hiding this comment

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

Awesome work, thank you!

@mauricioabreu mauricioabreu merged commit 85ac1bc into globocom:master May 9, 2024
7 of 8 checks passed
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.

None yet

3 participants