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

Improve CEA-608 column to VTT position #3270

Merged
merged 1 commit into from Dec 7, 2020

Conversation

robwalch
Copy link
Collaborator

@robwalch robwalch commented Dec 2, 2020

This PR will...

Calculate cue position for CEA-608 caption cues according to the PAC indent code table at https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/608toVTT.html#positioning-in-cea-608

Why is this Pull Request needed?

Improve rendering of 608 captions (multiline and left aligned).

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

Also a small optimization on whitespace check using RegEx.test rather than String.match.

Test streams:
https://playertest.longtailvideo.com/adaptive/captions/playlist.m3u8
https://playertest.longtailvideo.com/adaptive/cpcweb/cpcdemo.m3u8

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

@robwalch robwalch added this to the 1.0.0 milestone Dec 2, 2020
@robwalch robwalch requested a review from OrenMe December 2, 2020 19:14
@robwalch robwalch added this to Top priorities in Release Planning and Backlog via automation Dec 2, 2020
@robwalch robwalch moved this from Top priorities to v1.0.0 in Release Planning and Backlog Dec 2, 2020
OrenMe
OrenMe previously approved these changes Dec 2, 2020
@robwalch
Copy link
Collaborator Author

robwalch commented Dec 2, 2020

@OrenMe some snippets that can help with testing:

  1. "indent" to position output to compare to the PAC table in https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/608toVTT.html#positioning-in-cea-608 (hopefully what we refer to as indent in our code matches the the spec):
for (let i = 0; i < 29; i++) {
    console.log(i, Math.max(10, 10 + Math.min(80, Math.floor(i * 8 / 32) * 10)));
}
  1. This is more for visual testing of cue lines in native TextTrack rendering across browsers if you don't have a stream chock full of lines:
(function() {
  const video = document.querySelector('video');
  const track = video.addTextTrack('subtitles', 'English', 'en');
  track.mode = 'showing';
  const start = 1;
  const end = video.duration - 1;
  for (let i=15; i>12; i--) {
      const cue = new VTTCue(start, end, `Line ${i}`);
      cue.line = i;
      track.addCue(cue);
  }
}());

@OrenMe
Copy link
Collaborator

OrenMe commented Dec 2, 2020

I actually ran the code for #1 when reviewing it and I saw it is with line of what we saw in the spec.
#2 is great for testing and I saw it also in the chromium bug report - thanks!

I'm checking about fixing the long tail to the right which we saw.

@robwalch robwalch force-pushed the bugfix/cea-608-row-column-to-vtt branch from dc8d515 to e65e1a6 Compare December 4, 2020 18:52
@robwalch robwalch merged commit 2ac1c85 into feature/v1.0.0 Dec 7, 2020
@robwalch robwalch deleted the bugfix/cea-608-row-column-to-vtt branch December 7, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants