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

Bug in discardCuesBeforeTimeUs() in ReplacingCuesResolver.java #1939

Closed
1 task
wischnow opened this issue Dec 2, 2024 · 1 comment
Closed
1 task

Bug in discardCuesBeforeTimeUs() in ReplacingCuesResolver.java #1939

wischnow opened this issue Dec 2, 2024 · 1 comment
Assignees

Comments

@wischnow
Copy link
Contributor

wischnow commented Dec 2, 2024

Version

Media3 1.5.0

More version details

Tested with version 1.5.0, but this is a very long standing bug.

Devices that reproduce the issue

The bug is device-independent.

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

I have had a small patch for exoplayer/media3 which adds support for Vobsub subtitles. Evene though there I create CuesWithTiming objects with a durationUs of 5 seconds, the subtitles were never removed from display. I finally found the time to dig deeper and find the bug. Look at discardCuesBeforeTimeUs() in ReplacingCuesResolver.java:

  public void discardCuesBeforeTimeUs(long timeUs) {
    int indexToDiscardTo = getIndexOfCuesStartingAfter(timeUs);
    if (indexToDiscardTo > 0) {
      cuesWithTimingList.subList(0, indexToDiscardTo).clear();
    }
  }

Since getIndexOfCuesStartingAfter() only looks at startTimeUs (as the name implies), this will also remove cues whose endTimeUs has not been reached yet, so that reaching that time will not trigger removing the subtitle.

To fix this, I admit, I was lazy:

  public void discardCuesBeforeTimeUs(long timeUs) {
    int indexToDiscardTo = getIndexOfCuesStartingAfter(timeUs);

    if (indexToDiscardTo > 1) {
      cuesWithTimingList.subList(0, indexToDiscardTo - 1).clear();
    }
  }

A more thorough implementation would check whether the endTimeUs of that cue is in the past or not, but since getPreviousCueChangeTimeUs() and getNextCueChangeTimeUs() check the times correctly, being lazy works in this case.

It may well be that this bug is never triggered for the types of subtitles supported by media3 out of the box, but that code is still wrong.

Expected result

See the description above.

Actual result

See the description above.

Media

None available

Bug Report

@rohitjoins rohitjoins self-assigned this Dec 3, 2024
copybara-service bot pushed a commit that referenced this issue Dec 4, 2024
The method previously discarded the cue that was active at `timeUs`,
meaning it had started before but had not ended by `timeUs`.

Issue: #1939

#cherrypick

PiperOrigin-RevId: 702707611
@rohitjoins
Copy link
Contributor

Hi @wischnow,

Thank you for raising this issue, we have added a fix for this bug.

shahdDaghash pushed a commit that referenced this issue Dec 18, 2024
The method previously discarded the cue that was active at `timeUs`,
meaning it had started before but had not ended by `timeUs`.

Issue: #1939

PiperOrigin-RevId: 702707611
(cherry picked from commit e927d7b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants