-
Notifications
You must be signed in to change notification settings - Fork 131
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 bug: subtitle no longer blink in low latency #1314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Small nitpick on the PR message as it wasn't limpid:
It was due to the size of the subtitles segments that differs in normal mode vs low-latency
To note that this is not inherent to DASH low-latency. Packagers could theoretically have smaller or bigger segments/subsegments where the issue could be much rarer or not exist.
@@ -26,6 +26,11 @@ import { | |||
removeCuesInfosBetween, | |||
} from "./utils"; | |||
|
|||
const DELTA_CUES_GROUP = 1e-3; // 1ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could have a small JSDoc to describe its purpose once we forget the issue or somebody else look at it
src/core/segment_buffers/implementations/text/html/text_track_cues_store.ts
Show resolved
Hide resolved
src/core/segment_buffers/implementations/text/html/text_track_cues_store.ts
Show resolved
Hide resolved
// the following one if they are close enough | ||
// ours: |AAAAA| | ||
// the current one: |BBBBB|... | ||
// Result: |BBBBBBBAAAAA| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comments are weirdly sometimes (not the first time) strangely indented more than the code it comments. Is it on purpose?
On another subject, we could explain here that this allows to avoid having a very small gap without subtitles when that was probably not wanted.
Nice, it seems that the linter is angry though. You can check it locally by doing an |
57dab60
to
77c34ce
Compare
// or end time than the start or end time of the ICuesGroup due to parsing | ||
// approximation. | ||
// Add a tolerance of 1ms to fix this issue | ||
if (ret.length === 0 && cues.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should still be explicit about the length check here. I don't know why the linter isn't failing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed it, linter was fine with both ways
77c34ce
to
5e36a77
Compare
LGTM, waiting for the CI and then if it goes well, merging it |
The bug
When playing a video in low-latency mode with subtitle, the subtitle were disappearing and reappear very quickly making the subtitles "blink".
This was only experienced in low-latency mode.
Explanations
It was due to the size of the subtitles segments that differs in normal mode vs low-latency:
When sorting segments in the buffer, there is a logic that compare which segments should come first, and it determines if there are segments that are the "same" or if they overlap.
The logic that compute if two segments are the "same" were relying on a constant
MAX_DELTA_BUFFER_TIME
equals to 200ms.200ms of tolerance is fine for segments that lasts 2s, but for segments of 40ms in low-latency it's way too high, and segments of 40ms that were different were considered "same".
As a correction, the tolerance is variable and depends on the segment duration that is being inserted. It's equal to one fifth of the duration of the segment.
Additionally, there was another issue on parsing of the cues:
the
end: 278946541.9999333
of the cuesGroup has more precision than theend: 278946542.039
of the cuesElement.In very specific case, there can be
time < cuesGroup.end
andtime > cuesElement.end
.In such case the
cuesElement
is not displayed.Again adding a small tolerance fix the issue