fix: delay appending vtt subtitles depending on presence of discontinuity sequence and timestamp offset

[gkatsev]

This PR changes the way that timestamp offsets are stored in the media source engine. Instead of a single value, it now has a map of values. For DASH, it should contain a single value, which is the last timestamp offset available. For HLS, it'll keep track of the timestamp offsets per discontinuity sequence. If content is appended and we don't yet have a timestamp offset for that discontinuity sequence number, it'll defer creating the cues until the timestamp offset is set, otherwise, the times for the cues may not be correct.

Fixes #9470

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[gkatsev]

the PR build is failing because there's a leading space character but seems like I can't edit the PR title.

[avelad]

@shaka-bot test

[shaka-bot]

@avelad: Lab tests started with arguments:

• pr=9676

[shaka-bot]

Bundle Size Report for PR #9676

File	HEAD	Base	Diff
build_state.json	0.8 KiB (0.5 KiB)	0.8 KiB (0.5 KiB)	0.0 KiB
controls.css	23.3 KiB (4.9 KiB)	23.3 KiB (4.9 KiB)	0.0 KiB
demo.compiled.debug.js	239.2 KiB (48.2 KiB)	239.2 KiB (48.2 KiB)	0.0 KiB
demo.compiled.js	239.2 KiB (48.2 KiB)	239.2 KiB (48.2 KiB)	0.0 KiB
demo.css	169.9 KiB (27.7 KiB)	169.9 KiB (27.7 KiB)	0.0 KiB
locales.js	42.6 KiB (10.6 KiB)	42.6 KiB (10.6 KiB)	0.0 KiB
receiver.compiled.debug.js	143.2 KiB (23.7 KiB)	143.2 KiB (23.7 KiB)	0.0 KiB
receiver.compiled.js	143.2 KiB (23.7 KiB)	143.2 KiB (23.7 KiB)	0.0 KiB
shaka-player.compiled.debug.js	1491.3 KiB (340.8 KiB)	1490.4 KiB (340.5 KiB)	+0.9 KiB
shaka-player.compiled.js	747.3 KiB (241.9 KiB)	746.7 KiB (241.7 KiB)	+0.6 KiB
shaka-player.dash.debug.js	1090.6 KiB (254.6 KiB)	1089.7 KiB (254.3 KiB)	+0.9 KiB
shaka-player.dash.js	513.5 KiB (170.1 KiB)	512.9 KiB (169.8 KiB)	+0.6 KiB
shaka-player.experimental.debug.js	1873.5 KiB (420.9 KiB)	1872.7 KiB (420.6 KiB)	+0.9 KiB
shaka-player.experimental.js	982.8 KiB (306.4 KiB)	982.2 KiB (306.1 KiB)	+0.6 KiB
shaka-player.hls.debug.js	1127.8 KiB (260.9 KiB)	1126.9 KiB (260.6 KiB)	+0.9 KiB
shaka-player.hls.js	551.9 KiB (181.7 KiB)	551.3 KiB (181.5 KiB)	+0.6 KiB
shaka-player.ui.debug.js	1812.5 KiB (407.9 KiB)	1811.7 KiB (407.6 KiB)	+0.9 KiB
shaka-player.ui.js	956.5 KiB (298.4 KiB)	955.9 KiB (298.2 KiB)	+0.6 KiB

[shaka-bot]

Incremental code coverage: 90.77%

[avelad]

It seems that fails in some platforms... can you review it? Thanks!

[matvp91]

deferredAppends_ seems always truthy, making this condition dead code?

[gkatsev]

Good catch! It must've stayed from an earlier version where it wasn't always set.

[gkatsev]

So, I think the test failures happen if the subtitles take a while to load and the second video segment ends up loading first, which changes the timestamp offset when they're parsed. During my testing previously, this is basically the same behavior as shaka had before this change.
At one point I experimented with not letting a timestamp offset get overridden once set but I think there were some scenarios that weren't correct.
A way to force this to happen for the one test is to add || this.timestampOffsetMap_.get(disco) != -1.4 to the .has(disco) in appendBuffer.

[avelad]

So, I think the test failures happen if the subtitles take a while to load and the second video segment ends up loading first, which changes the timestamp offset when they're parsed. During my testing previously, this is basically the same behavior as shaka had before this change. At one point I experimented with not letting a timestamp offset get overridden once set but I think there were some scenarios that weren't correct. A way to force this to happen for the one test is to add || this.timestampOffsetMap_.get(disco) != -1.4 to the .has(disco) in appendBuffer.

That sounds like a hack with a magic number... I think the ideal solution is to find the root cause and fix it. Modifying the test to wait a certain amount of time is also a solution.

[gkatsev]

Oh, yeah, I didn't mean it was a solution but rather a way to reliably trigger the conditions for the test failure. I'll continue investigating this today.

[gkatsev]

ok, I think the root cause is the change in media source engine where previously the promise only got reset in resync which is only called in some cases, like when a disco sequence changes but now the timestamp offset always gets updated. I'll need to do some more testing on this.

[gkatsev]

It seems like having the timestamp offset only get set once per discontinuity makes the tests pass. Potentially, would need a way to unset it in the case of a resync call.
Going to continue looking into it and run some more tests on our streams to make sure that it doesn't cause other issues.

[gkatsev]

ok, I added a check for whether the timestamp offset should get updated or not. In my testing it seems to be working fine for both multiperiod DASH and HLS content with discontinuities.

One thing to thing I noticed is that in DASH, when seeking back across a period, shaka inserts the init segment from the period we seeked away from resulting in the captions being offset. It doesn't always happen but it isn't hard to reproduce. This PR doesn't address that issue. While it may be possible to work around it on the text engine/MSE side, it might be nice to figure out why the incorrect init segments gets appended.

[gkatsev]

now that the manifest type is passed here, we technically don't need to pass it in initParser below. I didn't make that change yet, but happy to do so.

[avelad]

Please do it ! Thanks!

[avelad]

@shaka-bot test

[shaka-bot]

@avelad: Lab tests started with arguments:

• pr=9676

[gkatsev]

looks like there's stil more work for me https://github.com/shaka-project/shaka-player/actions/runs/22002953987/job/63583998586?pr=9676#step:13:365 😭

[gkatsev]

running the test locally in safari via karma's debug page seems to pass. It's off by exactly the first timestamp offset still.

[gkatsev]

I can reproduce when running karma on safari directly, so, I guess I'll take another look at it next week.

[gkatsev]

i think the root cause of the new failure is this change. While it seems to work for the most part, it means that there's a chance that we get text segments appended before a proper timestamp offset is set from the media source engine. It was initially added because a bunch of tests were failing and that worked around it. Instead, probably need to remove it and fixup the tests.