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: remove unnecessary handling of invalid cues #7956

Merged
merged 1 commit into from
Feb 1, 2023

Conversation

mister-ben
Copy link
Contributor

Description

We had some special handling of cues which have equal start and end times, so they would be considered active 0.5 after they start/end. Cues with equal start and end times are not valid; the end time must be greater. However some painton style VTT in the wild contain many such cues, resulting in all such with a 0.5 seconds being displayed.

We thought this handling might still be needed for ID3 cues as ID3 only has a start time, but it's not: on adding to the track the endTime is set to the next cue's startTime or the video duration.

Specific Changes proposed

Removes the handling of these cues. A cue with equal start and end times is not strictly ignored, but it will only render if active cues are updated at the precise millisecond they are to be active.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #7956 (78a00a8) into main (ce1baba) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #7956      +/-   ##
==========================================
+ Coverage   80.94%   80.99%   +0.05%     
==========================================
  Files         116      116              
  Lines        7467     7467              
  Branches     1816     1814       -2     
==========================================
+ Hits         6044     6048       +4     
+ Misses       1423     1419       -4     
Impacted Files Coverage Δ
src/js/tracks/text-track.js 94.87% <ø> (+1.74%) ⬆️
src/js/extend.js 85.71% <100.00%> (+3.36%) ⬆️
...rc/js/control-bar/progress-control/time-tooltip.js 84.61% <0.00%> (+2.56%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@OwenEdwards
Copy link
Member

Hmm, interesting. I hadn't thought about this before, and it's a separate (but related) issue - does the current way of handling text track cues work correctly for cues whose duration is less than the rate that this display method is called?

For example, if the timerupdate event is triggered every 1/4th of a second (I think that's what it is, or at least it used to be), and if a cue's end time is, say, 150mS after its start time, then it'll be entirely unpredictable whether that cue is displayed for 250mS or not at all, depending on where the timerupdate ticks occur in relation to the cue start time. Is that the right behavior? Is there anything in the text track spec about that? Just wondering 🤔

@gkatsev
Copy link
Member

gkatsev commented Oct 15, 2022

Given that @mister-ben added requestVideoFrameCallback/requestAnimationFrame support (with fallback), it's likely going to get called enough in most cases.
That said, the HTML spec says to skip missed cues.

Actually, while we're here, I realized we should make the end time exclusive.

@mister-ben
Copy link
Contributor Author

mister-ben commented Oct 17, 2022

Anything like adding time to caption durations to ensure they are displayed longer would only cause issues with unintended overlaps. There has to be an onus on the vtt authoring to ensure timings are long enough with a margin of error - no implemention could be more precise than the monitor refresh rate.

@mister-ben mister-ben added the patch This PR can be added to a patch release. label Oct 26, 2022
@mister-ben mister-ben merged commit db882cd into videojs:main Feb 1, 2023
edirub pushed a commit to edirub/video.js that referenced this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants