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 MPD Timing (remove publishTime & presentationTimeOffset) #564

Merged
merged 3 commits into from Apr 8, 2021
Merged

Fix MPD Timing (remove publishTime & presentationTimeOffset) #564

merged 3 commits into from Apr 8, 2021

Conversation

matthuisman
Copy link
Contributor

@matthuisman matthuisman commented Dec 2, 2020

Fixes: #560
Fixes: #615
Fixes: d21spike/plugin.video.sling#22

dc7b427 seems to have broken quite a few streams.
This PR basically reverts that

Looks like the problem is that it is using publishTime as a basis to calculate segment numbers.
"MPD@publishTime is merely a version label. The value is not used in timing calculations."
This PR reverts back to using dash->stream_start_ which is based off kodis clock.

Confirmed fix for BBC: #560 (comment) (publishtime)
Confirmed fix for LA7: #615 (comment) (publishtime)
Confirmed fix for Bein Mena (needed publishtime and presentationTimeOffset removed)
Confirmed fix for Sling: d21spike/plugin.video.sling#22 (comment)

@matthuisman matthuisman changed the title gnore presentationTimeOffset Ignore presentationTimeOffset Dec 2, 2020
@matthuisman matthuisman changed the title Ignore presentationTimeOffset Revert [DASH] Adjust timing calculation Mar 26, 2021
@matthuisman matthuisman changed the title Revert [DASH] Adjust timing calculation WIP: Fix MPD Timing Mar 29, 2021
@matthuisman matthuisman marked this pull request as draft March 29, 2021 03:49
@matthuisman matthuisman changed the title WIP: Fix MPD Timing Fix MPD Timing (remove publishTime) Mar 29, 2021
@matthuisman matthuisman marked this pull request as ready for review March 29, 2021 20:59
@matthuisman
Copy link
Contributor Author

matthuisman commented Mar 30, 2021

ok, new build with presentationTimeOffset removed as Bein Mena was still broken.

https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Finputstream.adaptive/detail/PR-564/18/artifacts

@CaptainTK
can you please test above build with BBC (and anything else using live MPD's)

@Testato @fawkes15 @light-wizard
can you please test above build with LA7 (and anything else using live MPD's)

@eracknaphobia @ckuyehar @d21spike
can you please test above build with Sling on Matrix (and anything else using live MPD's)

@light-wizard
Copy link

Tested android aarch64 build with LA7: all functions working.
Android 8, Kodi 19.
Thanks for the fix @matthuisman

@Testato
Copy link

Testato commented Mar 31, 2021

On LA7 all work, the two Live MPD and also the On-Demand MPD programs
Tested on Firestick

@matthuisman matthuisman changed the title Fix MPD Timing (remove publishTime) Fix MPD Timing (remove publishTime & presentationTimeOffset) Mar 31, 2021
@glennguy
Copy link
Contributor

glennguy commented Apr 1, 2021

Before bringing this in it would be good to confirm that presentationTimeOffset is definitely not needed in any segment calculations. We've got some tests now sure, but I'd like to review the DASH-IF guidelines one more time to see what the intended purpose is.

I can't recall exactly but I remember getting the impression previously that PTO might only be used for calculating where segments are in regard to program time for purposes of a visual timeline like Kodi's OSD - example: 2 hours into a 4 hour live event with a 30 minute buffer and being able to tell the player to show 1:30:00 as the start of the timeline, 2:00:00 as the end. In this case I believe there's nothing in the inputstream interface for passing on this information anyway. Someone please correct if that's not the case.

On the other hand it is possible that PTO is used in calculations, just that most often it is nullified by another value of similar or same size that is subtracted/added to bring back to a net or near zero.

This is just me thinking out aloud, I'll look into properly over the weekend.

@matthuisman
Copy link
Contributor Author

matthuisman commented Apr 1, 2021

Sounds good. The test mpd is one I know fails without ignoring pto.

this PR should be our next top priority for merging as its fixing regressions from 2.4.6.

And maybe the live / vod fix too.

Once they are merged. I'll PR for Leia this and the other regression fixes.

Still waiting to hear test results from Sling users / dev

Oh... And I remember seeing other PTO code somewhere too.

Here it is (segment list attribute)

else if (strcmp((const char*)*attr, "presentationTimeOffset") == 0)

@CaptainTK
Copy link

Apologies, I did not get to test this the last days.

https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Finputstream.adaptive/detail/PR-564/12/artifacts

looks empty to me, so if there is a newer version to test, let me know, please.

@matthuisman
Copy link
Contributor Author

@CaptainTK
Copy link

Thanks for that! Looks good to me, played around with a number of live channels, all worked well.

@ckuyehar
Copy link

ckuyehar commented Apr 2, 2021

@eracknaphobia @ckuyehar @d21spike
can you please test above build with Sling on Matrix (and anything else using live MPD's)

@matthuisman - i'd like to test but im unsure how to test this fix - any quick guide/setup document to setup the test?

@matthuisman
Copy link
Contributor Author

Sling fix confirmed:
d21spike/plugin.video.sling#22 (comment)

Sling also requires the live fix:
#626

@d21spike
Copy link

d21spike commented Apr 3, 2021

@matthuisman I appreciate the hard work!

@glennguy
Copy link
Contributor

glennguy commented Apr 7, 2021

Based on my understanding, PTO is meant to be used to help align video/audio/text streams where the segments aren't aligned.
This is an example case where the start of a period is part way through a segment (in orange):
image

Consider now that this is for video segments, but in the case for audio segments in this playlist they are perfectly aligned to the period start. PTO will signal to the player that the start of the period is 8220 on the sample timeline, player will offset accordingly into the segment. Basically it allows sub-segment alignment. Please correct if I'm wrong in this.

Two things:

  • IA only supports a simple list of segments that belong to a representation/period, there is currently no way to give a time or sample offset to apply a subsegment alignment.
  • There's no open issues that relate to out of sync audio/video streams. I can't be sure but I imagine VideoPlayer already takes care of aligning PTS between audio/video streams.

In other words, I don't believe presentationTimeOffset is required to be observed at the moment. In all cases so far we have enough information elsewhere to correctly calculate the segment numbers.

The changes fix broken streams while preserving existing ones that we're aware of, so LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants