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(DASH): Parse and use target latency #6683

Merged
merged 2 commits into from
May 29, 2024

Conversation

avelad
Copy link
Member

@avelad avelad commented May 28, 2024

Related to #6193
Thanks to @gkatsev

@avelad avelad added type: enhancement New feature or request priority: P3 Useful but not urgent component: DASH The issue involves the MPEG DASH manifest format labels May 28, 2024
@avelad avelad added this to the v4.9 milestone May 28, 2024
@avelad avelad requested review from joeyparrish and theodab May 28, 2024 09:06
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 64.44%

Copy link
Contributor

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Thanks! Is there a reason for not adding the target latency as on option as well? Or just want that to be as part of the updated #6193?

@avelad
Copy link
Member Author

avelad commented May 28, 2024

Thanks! Is there a reason for not adding the target latency as on option as well? Or just want that to be as part of the updated #6193?

By rereading again, then min and max stop making sense, because we would always use target if we use liveSync = true.

So there are two options:

  • Do this which is the base and do nothing with the other.
  • Do this which is the base and deprecate min and max through config and always use target + tolerance.

Since in both options you have to do this PR, I have gone for the simple thing.

But I would like to hear your opinion.

@gkatsev
Copy link
Contributor

gkatsev commented May 28, 2024

It's a bit tricky. From the DASH spec, the only property that's required is target latency. From my understanding, the DASH spec is also a bit stricter in terms of what max and min latency mean, where playback should happen in those areas (potentially, triggering a seek to the target latency?).
I don't have concerns with this PR as is, but it would be worth ironing out how shaka wants to handle these properties and work towards that. This change is a good first step, regardless of the following direction.

@avelad
Copy link
Member Author

avelad commented May 28, 2024

It's a bit tricky. From the DASH spec, the only property that's required is target latency. From my understanding, the DASH spec is also a bit stricter in terms of what max and min latency mean, where playback should happen in those areas (potentially, triggering a seek to the target latency?). I don't have concerns with this PR as is, but it would be worth ironing out how shaka wants to handle these properties and work towards that. This change is a good first step, regardless of the following direction.

After this PR I will work on making the configuration simpler, so I will add liveSyncTargetLatency and remove liveSyncMaxLatency and liveSyncMinLatency. Thanks for you point of view

@avelad avelad merged commit 9060ab0 into shaka-project:main May 29, 2024
15 checks passed
@avelad avelad deleted the target-latency branch May 29, 2024 09:08
avelad pushed a commit that referenced this pull request Jun 17, 2024
…cy and liveSyncMaxLatency options (#6822)

Follow-up from #6683
to add a target latency config and deprecate the existing min/max
options.
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jul 28, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jul 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: DASH The issue involves the MPEG DASH manifest format priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants