Skip to content

Conversation

@robwalch
Copy link
Collaborator

@robwalch robwalch commented Nov 5, 2025

This PR will...

  • Fix LL-HLS VTT subtitle part loading
  • Fix fragment selection when live subtitle playlist does not overlap with position
  • Fix subtitle selection in asset players while primary is detached
  • Remove dead code in VTT parser, and remove redundant code in subtitle-stream-controller

Why is this Pull Request needed?

These changes unify subtitle segment finding with audio and video, fixing issues with subtitle part and segment loading.

Are there any points in the code the reviewer needs to double check?

Low-latency live with VTT parts https://llhls-demo.ovenmediaengine.com/app/stream/llhls.m3u8

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

Fix fragment selection when live subtitle playlist does not overlap with position (#7557)
Fix subtitle selection in asset players while primary is detached
Copy link
Collaborator

@hongjun-bae hongjun-bae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR helps address the issue related to the previous track=undefined setting and the lenient subtitle controller.

);
if (!foundFrag) {
this.warn('Subtitle playlist not aligned with playback');
track.details = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for streaming subtitles in LL-HLS has become much more lenient 👍

Copy link
Collaborator Author

@robwalch robwalch Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional changes in #7482 will help to ensure proper playlist alignment. When combined with these changes, audio and subtitle segment loading will follow more of the same logic.

}
if (this.waitForLive(track)) {
const { currentTrackId, levels } = this;
const track = levels?.[currentTrackId];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before there was a check to see if track is undefined, now it's removed here. I believe this may cause issues. We may need to keep what was here previously:
if (!track || !levels.length || !track.details) { return; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're covered by track?.details and !trackDetails. No details, no track.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. that's true that the original code had that: !track.details

{
private currentTrackId: number = -1;
private tracksBuffered: Array<TimeRange[]> = [];
private tracksBuffered: Array<TimeRange[] | undefined> = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anywhere where we are appending an undefined object to tracksBuffered? Do we suspect we need this?

Copy link
Collaborator Author

@robwalch robwalch Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't append undefined values, but we do not use noUncheckedIndexedAccess either. When using (@typescript-eslint/) no-unnecessary-condition (I use this offline, but not in the project proper), typing this way helps by signaling that the result to tracksBuffered[<missing-key-or-index>] may be undefined. Yes, it is bad typing, but simpler safety. I'm open to suggestions - short and long term.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I think this is fine for now. And agree maybe something to look into in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

4 participants