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

fix bug: subtitle no longer blink in low latency #1314

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

Florent-Bouisset
Copy link
Collaborator

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:

  • Normal mode: around 2 seconds
  • Low latency: around 2 seconds with chunks of 40 milliseconds

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 the end: 278946542.039 of the cuesElement.
In very specific case, there can be time < cuesGroup.end and time > cuesElement.end.
In such case the cuesElement is not displayed.
Again adding a small tolerance fix the issue

{
    "start": 278946541.9999333,
    "end": 278946542.0399333,
    "cues": [
        {
            "start": 278946541.999,
            "end": 278946542.039,
            "element": {}
        }
    ]
}

Copy link
Collaborator

@peaBerberian peaBerberian left a 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
Copy link
Collaborator

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

// the following one if they are close enough
// ours: |AAAAA|
// the current one: |BBBBB|...
// Result: |BBBBBBBAAAAA|
Copy link
Collaborator

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.

@Florent-Bouisset Florent-Bouisset added bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 2 (Medium) This issue or PR has a medium priority. labels Nov 14, 2023
@peaBerberian
Copy link
Collaborator

Nice, it seems that the linter is angry though. You can check it locally by doing an npm run lint?

// 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) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@peaBerberian peaBerberian added this to the 3.33.0 milestone Nov 14, 2023
@peaBerberian peaBerberian added Priority: 1 (High) This issue or PR has a high priority. and removed Priority: 2 (Medium) This issue or PR has a medium priority. labels Nov 14, 2023
@peaBerberian
Copy link
Collaborator

LGTM, waiting for the CI and then if it goes well, merging it

@peaBerberian peaBerberian merged commit 9a3c204 into master Nov 14, 2023
4 checks passed
@peaBerberian peaBerberian mentioned this pull request Jan 24, 2024
@peaBerberian peaBerberian deleted the bugfix-subtitle-v3 branch February 7, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Priority: 1 (High) This issue or PR has a high priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants