apply periodStart instead of timestampOffset to text reference times

[baconz]

The start/end times are derived from reference times which have
already had the offsets applied. Without this patch, when there
is an offset present we buffer all available text segments at
stream start.

[googlebot]

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).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[baconz]

I think there is still an issue here.

Here's how I understand the code:

The timestamp offset that is set on the engine is the offset relative to the start of the period. In other words, it is the offset required to calculate the presentation time within the period; the first segment of the period should be 0 in this world.

The calls to evict and subsequently to remove, use time relative to the availability start time, which has an entirely different time scale. Since the cue/buffer times are relative to the period, bad things seem to happen on the evict calls.

I think this patch fixes an issue whereby single-period playback won't start until all text fragments have been buffered, but multi-period playback is still broken if there is a text track.

Update I looked at this again, and I stand corrected:

The issue is that the startTime and endTime passed to appendBuffer are relative to the start of the period (pto), while everything else is relative to the availability start time via timestampOffset

One solution would be to pass the periodStart all the way through streaming_engine to text_engine and adjust the buffer start/end accordingly. I'm guessing that's more tight-coupling than you guys would be comfortable with. Other ideas?

[joeyparrish]

The original code looks correct to me. If we are emulating MSE's timestamp offset, we should add the offset to the parsed timestamps. Can you please give us a sample manifest that reproduces your issue?

[baconz]

Sorry, the PR is misleading, I'm not suggesting we remove the offset, just that the offset be relative to the AST, as opposed to the period start.

Here's a sample manifest. I can make playback available if you want to get in touch with me out of band.
manifest.txt

[baconz]

Let me try to be explicit about what I believe that I'm seeing in the text flow. I'm glossing over many parts of the system, but this is how I understand it:

1. References are created in SegmentList.parseSegmentListInfo_ and have the PTO applied to their start and end times. Now all the references have times relative to the period start, not the AST.
2. StreamingEngine decides the references need to be buffered, and we end up in fetchAndAppend, which calls initSourceBuffer on the current period
3. We set the timestampOffset on the media source to period start - PTO. The timestampOffset is relative to the AST. Please correct me if I'm wrong about this assumption.
4. Once we fetch the segments they get passed through to StreamingEngine.append_, which calls StreamingEngine.appendBuffer_ using the start and end times of the references. Per my assumption above, these times are period relative.
5. Now we arrive at TextEngine.appendBuffer, which is adding the offset to the the reference start/end times. As I see it, the math we are doing is:

referenceTime = origTs - PTO
offset = periodStart - PTO
newReferenceTime = referenceTime + offset

where referenceTime is startTime or endTime in TextEngine.appendBuffer. This can be rewritten as

newReferenceTime = origTs - PTO + periodStart - PTO

So we're subtracting out the PTO twice. I think what we actually want is:

newReferenceTime = origTs - PTO + periodStart

I think the right solution is to pass the periodStart into TextEngine so that we can add it to the reference times that we pass in to appendBuffer.

I'm happy to move this off to an issue, if you think it is legit.

[joeyparrish]

Here's why this is difficult to understand. MediaSourceEngine delegates to TextEngine for text streams because MediaSource doesn't handle them. So TextEngine is supposed to work just like MediaSource/SourceBuffer. TextEngine's timestampOffset is meant to work exactly like SourceBuffer's timestampOffset: it is added to the timestamps of the media contained in future calls to appendedBuffer().

So it's not at all clear to me why this should need to be different for text.

In the manifest you attached, your text streams have a presentationTimeOffset identical to the ones in your audio and video streams from the same Period. Do the timestamps in the text segments align with the ones in the corresponding video segment?

Can you send a manifest URL I can use to see this in action?

[baconz]

I agree that we should be adjusting the timestamps of the media passed to TextEngine.appendBuffer by timeStampOffset. That part of the code is behaving correctly (https://github.com/google/shaka-player/pull/411/files#diff-564eace42ce079927d4fbedc838cf4aeR129).

The issue is that we are using the reference start/end time (which are not passed to a normal MediaSource) to adjust bufferStart_ and bufferEnd_ (https://github.com/google/shaka-player/pull/411/files#diff-564eace42ce079927d4fbedc838cf4aeL123). This happens automagically inside of MediaSource, but we need to do it manually for TextEngine. My point is that those start and end times are getting the PTOs applied twice.

Sorry this is dragging out. I don't have a great way to make that content to you available publicly, but we might be able to make something work out of band...

[joeyparrish]

Ah, okay, I see now. It's the buffered ranges that are wrong.

[joeyparrish]

I'm totally on board with this change now. If you would, please:

1. Fix the commit author to satisfy the CLA bot
2. Update the title of the commit/PR to make it clearer what's going on (something like "do not apply timestampOffset to text reference times")
3. Update the TextEngine unit tests in test/text_engine_unit.js (there's a section called "bufferStart/bufferEnd", and it looks like some of these would fail with your PR as-is)

You can easily run the tests with ./build/test.py

Thanks!

[baconz]

I passed periodStartall the way through to TextEngine. I think an alternative would be to add periodStart to the reference object, and create methods to calculate the AST-relative start and end times, but that seemed like a bigger change, so I figured best to start here.

Let me know what you think.

[joeyparrish]

We have repro now and a new test asset to cover this case, but we have another fix in mind.

[baconz]

Sounds good. Let me know if you need anything else from me.

[joeyparrish]

@baconz Please try the latest from master and confirm whether this fixes it for you.

[baconz]

👍 It works! Thanks, Joey!