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

PTS rollover handling incorrectly applied to DASH and fMP4 HLS #2808

Closed
NoChance777 opened this issue Aug 19, 2020 · 6 comments
Closed

PTS rollover handling incorrectly applied to DASH and fMP4 HLS #2808

NoChance777 opened this issue Aug 19, 2020 · 6 comments
Labels
component: HLS The issue involves Apple's HLS manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@NoChance777
Copy link
Contributor

Have you read the FAQ and checked for duplicate open issues?
Yes

What version of Shaka Player are you using?
3.0.3 (shaka 2.5.x release is working without the issue)

Can you reproduce the issue with our latest release version?
Yes

Can you reproduce the issue with the latest code from master?
Yes

Are you using the demo app or your own custom app?
I have used the demo app

If custom app, can you reproduce the issue using our demo app?
Yes

What browser and OS are you using?
Chrome/MacOS

For embedded devices (smart TVs, etc.), what model and firmware version are you using?

What are the manifest and license server URIs?
Have been sent by email

What did you do?
Start playing DASH stream with embedded WebVTT subtitles

What did you expect to happen?
subtitles should be displayed

What actually happened?
With DASH or HLS with MPEG-TS, the maximum PTS is 2^33 = 8589934592 . There are packager on the market (eg. Harmonic), which are messing up this rollover within the vtt files. There are always several .vtt files arriving with values above 8589934592, before the rollover is working. This is a bug on a packager side, and I guess you are trying to work around this bug. We are doing the same in our packager code, but only for MPEGTS input.
With DASH or HLS with fMP4 (that’s what we are using), this limit does not exist, but the rollover code is still applied.

Commit that brings this bug
W3 docs

@michellezhuogg michellezhuogg self-assigned this Aug 22, 2020
@joeyparrish joeyparrish changed the title PTS rollover handling, which is only valid for HLS with TS segments. This is completely messing up the PTS handling for DASH and HLS with fMP4 PTS rollover handling incorrectly applied to DASH and fMP4 HLS Oct 12, 2020
@TheModMaker
Copy link
Contributor

The problem is that TS segments will always have a small timestamp due to the rollover problem. To fix this, we adjust the WebVTT timestamps backward so they appear at the same time relative to the TS segment. You can also have both TS and MP4 in the same manifest (though we won't switch between them). This can complicate where the segments and text samples should be put.

What we should be doing is adjusting the TS segments forward so they appear at the correct time even though their timestamps aren't big enough to hold the correct time. So if a TS segment says it should be at 4 seconds, we may need to add the rollover so it appears after the previous rollover. We can use the WebVTT timestamps to find where to adjust the first segments.

I also wonder if we handle mid-stream rollovers correctly. So if we have a segment with a large timestamp and the next segment has a small one, will it work. The browser is supposed to handle this while playing, but this can complicate seeking.

@TheModMaker TheModMaker added type: bug Something isn't working correctly component: HLS The issue involves Apple's HLS manifest format and removed needs triage labels Oct 27, 2020
@TheModMaker TheModMaker added this to the v3.1 milestone Oct 27, 2020
@joeyparrish joeyparrish added the priority: P2 Smaller impact or easy workaround label Apr 19, 2021
@TheModMaker
Copy link
Contributor

I had intended to fix this for v3.1, thinking I could just revert that change and it would work out. But unfortunately, we no longer parse segment times for WebVTT, so reverting that change wouldn't do what I wanted. This means that there is no simple fix for this issue and we'd need to do the "full" fix for handling TS timestamps.

Since this change would be a large change and might introduce subtle bugs, we're going to wait until v3.2 to make this change. We may be able to put this in a v3.1.x bugfix release, depending on how clean the fix is.

@TheModMaker TheModMaker modified the milestones: v3.1, v3.2 Apr 19, 2021
@joeyparrish joeyparrish modified the milestones: v3.2, v3.3 Jul 7, 2021
@TheModMaker TheModMaker removed their assignment Oct 18, 2021
@xoundboy
Copy link

I'd like to re-open this discussion if I may.

So far we have been able to work around this problem by sticking to Shaka version 2.5.x for our web players and CC SDK v2 cast receiver. However we are now under pressure from Google to prepare a new Chromecast receiver based on the CAF v3 framework, which as you know is currently embedding Shaka Player version 3.0.10.

In other words, subtitles are broken in our new cast receiver because of this issue. We would also like to finally upgrade to version 3.x of Shaka for all platforms as soon as possible.

We have looked into possible fixes on the backend for this, but any of the possible workarounds identified there would be very complicated to implement, not to mention risky.

@joeyparrish What would be the best way to expedite this fix? Is there anything we can do to help? I understand from what @TheModMaker said in his previous comment here, that the fix isn't trivial. Is there any chance of this fix making it into general release soon?

Thanks in advance.

@joeyparrish
Copy link
Member

I can't promise a timeline since we don't currently have a fix. You can always contribute, though, if you have an idea and want to send a PR. That's the best way to expedite getting the fix you want, because it's entirely in your control. In case that is not feasible for you for any reason, I'll increase the priority to P1, though, so it's higher on the team's list. We apologize for any inconvenience.

@joeyparrish joeyparrish added priority: P1 Big impact or workaround impractical; resolve before feature release and removed priority: P2 Smaller impact or easy workaround labels Oct 26, 2021
@avelad avelad modified the milestones: v3.3, v4.1 May 4, 2022
@avelad
Copy link
Collaborator

avelad commented May 17, 2022

I don't know if this is resolved with the latest changes in HLS, @joeyparrish , can you check it?

@joeyparrish
Copy link
Member

I believe this is resolved now since #4217.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: HLS The issue involves Apple's HLS manifest format priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

7 participants