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

Fix: Allow the playback whenever the t attribute is missing in the SegmentTimeline elements #4174

Merged
merged 3 commits into from Apr 25, 2023

Conversation

dario-fiore
Copy link
Contributor

Hello team, I would like to submit some changes to add support for the playback of dynamic manifests whenever the t attribute is missing in the SegmentTimeline elements.

manifest

With this change, the player now handles the case when segments[0].t is NaN and calculates the timeshift window using the default range.start and the timeShiftBufferDepth attribute of the manifest.

This update could also improve the reliability of the player and align the behavior with all the other commercial players. Please review and provide your feedback. Thank you!

@dsilhavy
Copy link
Collaborator

@dario-fiore Thanks for your PR. Following our contribution guidelines, and since this seems to be your first PR, could you please send me a signed copy of dash.js feedback agreement?

@dsilhavy
Copy link
Collaborator

Thanks @dario-fiore .

We should indeed cover the case when S@t is missing. From 23009-1: If not present, then the value shall be assumed to be zero for the first S element.

In _calcRangeForTimeline we avoid using the timeshiftBufferDepth on purpose. The goal is to use the Segments in SegmentTimeline to determine the DVR window.

I think we can still use the summed duration here:

 if(!isNaN(segmentTime)) {
    range.end = calcPresentationTimeFromMediaTime((segmentTime + d) / timescale, voRepresentation);
} else {
     range.end = timeShiftBufferDepth;
}

So instead of using timeShiftBufferDepth we set segmentTime to 0 if it is NaN (due to missing @t).

What do you think? Also do you have a testvector you can share?

@dsilhavy dsilhavy added this to the 4.7.1 milestone Apr 24, 2023
@dario-fiore
Copy link
Contributor Author

dario-fiore commented Apr 24, 2023

Yes, it makes sense (definitely better to use the segments in the timeline). I just pushed the updated version.

Regarding the testvector here you can find a sample: https://gist.github.com/dario-fiore/709c1c71f6a25a174a8d3df1dd1ee739. Unfortunately, I can't share the whole stream.

Do you think it is possible to include this change in 4.7.0?

@dsilhavy dsilhavy modified the milestones: 4.7.1, 4.7.0 Apr 25, 2023
@dsilhavy dsilhavy merged commit 377d60a into Dash-Industry-Forum:development Apr 25, 2023
3 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

2 participants