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 for TTML text alignment #573

Merged
merged 6 commits into from
Nov 7, 2016
Merged

Conversation

birme
Copy link
Contributor

@birme birme commented Nov 4, 2016

This pull request solves the problem with TTML text alignment. We have TTML subtitles that has tts:textAlign=left and the subtitle was placed on the left side of the player but the text was centered. If I am reading the VTTCue specification correctly [1] the cue.align also needs to be set. This was not done and is solved by this PR.

[1] https://w3c.github.io/webvtt/#dom-vttcue-align

Copy link
Contributor

@ismena ismena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good otherwise.

@@ -160,12 +160,13 @@ shaka.media.TtmlTextParser.percentValues_ = /^(\d{1,2}|100)% (\d{1,2}|100)%$/;
*/
shaka.media.TtmlTextParser.lineAlignConvTable_ = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call it "textAlignToLineAlign_" instead so it's clear what we are mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

@@ -418,6 +418,25 @@ describe('TtmlTextParser', function() {
'</tt>');
});

it('aligns center when textAlign is center', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this test and a previous one into a single it('parses cue alignment from textAlign attribute') with two calls to verifyHelper inside it. (The same way 'disregards empty divs and ps' does 3 calls).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done!

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@ismena ismena merged commit f7fd3c9 into shaka-project:master Nov 7, 2016
@ismena
Copy link
Contributor

ismena commented Nov 7, 2016

Merged!

// Need to set cue.position to auto to get centered
// subtitles
cue.align = 'middle';
cue.position = 'auto';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, Jonas, I missed this when I reviewed: what is the rationale between setting position if textAlign is center?
I might have to roll this back for now because it messes with Safari and Firefox tests (these browsers don't support 'auto' value for VTTCue.position), but I can totally address this and reintroduce setting position if there's a valid reason to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem was that cue.align on Chrome does not support setting it as "center" though it should be valid. And using cue.align as "middle" doesn't have any effect without setting cue.position as auto. According to spec "auto" or a number should be a valid setting. If you can get it to center it on Chrome in another way I am happy to understand how. What happens without cue.position=auto is that the text is centered but on the left side of the screen. The most correct way probably but not supported from what I can see is to use VTTRegion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you. Let me file some bugs :)
Do I understand the problem with Chrome correctly: cue.align = 'center' is not supported and setting cue.align = 'middle' does not result in text being positioned in the middle of the cue box?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! (And fixed the tests)
Links to the bugs for references:
http://crbug.com/663797
https://bugzilla.mozilla.org/1316134

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A dev from Chrome got back to me asking for a vtt sample got the 'middle' bug. They're about to change it to 'center', but are having hard time reproducing the case where it doesn't position text in the center. Do you think you could share a sample that has the problem?
You can email it to me at ismena@google.com

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great that they could confirm the cue.align issue. When thinking about this a bit more the cue.position might be working as intended however the difficult thing is that cue.position has a centered anchor when cue.align is middle/centered and that might very well be according to spec. And when cue.align is left aligned the cue.position anchor is to the left. In TTML they have separated this where you can define a cue box region with a size and position. For example position 2 would put the region 2 units from the left. And then having textAlign=centered would place the text centered within that box. So, when translating this from a TTML space to a VTT space I had to set position as "auto" to give the same effect.

If I have understood the vtt spec a similar concept is specified with VTTRegion however that is not yet supported in Chrome as far as I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to answer the sample request. I don't think it will be possible to share due to drm and geoblocking issues. However if you for example set cue.align=middle and cue.position=2 I believe you would get the same effect as some TTML would produce. Again, not necessarily that part is a bug in Chrome and maybe how it is intended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I believe you're right and that might be the intended behavior (cue.position governs the position of the cue itself while align is about positioning the text inside the cue).
Thanks. I'll work with the Chrome team on fixing the align='center' issue.

joeyparrish pushed a commit that referenced this pull request Nov 30, 2016
Parse TTML textAlign settings into align property of a VTTCue.
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants