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.

Sign up for GitHub

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

Fix for TTML text alignment #573

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e973913
Fix to handle TTML textAlign
birme Nov 4, 2016
ba7b5f0
No need to use a function for the lineALign conversion. Use const ins…
birme Nov 4, 2016
d53ed72
Fix incorrect return type for lineAlignConvTable
birme Nov 4, 2016
f8a65a9
Handle center alignment
birme Nov 4, 2016
b1ef085
Merged the two align tests into one with two calls to verifyHelper in…
birme Nov 5, 2016
e698ca9
Renamed the linealign mapping table to textAlignToLineAlign
birme Nov 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions lib/media/ttml_text_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,19 @@ shaka.media.TtmlTextParser.timeHMSFormat_ =
shaka.media.TtmlTextParser.percentValues_ = /^(\d{1,2}|100)% (\d{1,2}|100)%$/;


/**
* @const
* @private {!Object}
*/
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!

'left': 'start',
'center': 'center',
'right': 'end',
'start': 'start',
'end': 'end'
};


/**
* Gets leaf nodes of the xml node tree. Ignores the text, br elements
* and the spans positioned inside paragraphs
Expand Down Expand Up @@ -274,10 +287,6 @@ shaka.media.TtmlTextParser.addStyle_ = function(
var TtmlTextParser = shaka.media.TtmlTextParser;
var results = null;

var align = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'tts:textAlign');
if (align)
cue.lineAlign = align;

var extent = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'tts:extent');
Expand Down Expand Up @@ -317,6 +326,20 @@ shaka.media.TtmlTextParser.addStyle_ = function(
}
}
}

var align = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'tts:textAlign');
if (align) {
if (align == 'center') {
// 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.

} else {
cue.align = align;
}
cue.lineAlign = TtmlTextParser.lineAlignConvTable_[align];
}
};


Expand Down
40 changes: 40 additions & 0 deletions test/media/ttml_text_parser_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,44 @@ describe('TtmlTextParser', function() {
'end="01:02:03.200">Line1<br/>Line2</p></body></tt>');
});

it('aligns left when textAlign is left', function() {
verifyHelper(
[
{start: 62.05, end: 3723.2, text: 'Test', lineAlign: 'start',
align: 'left'}
],
'<tt xmlns:tts="ttml#styling">' +
'<styling>' +
'<style xml:id="s1" tts:textAlign="left"/>' +
'</styling>' +
'<layout xmlns:tts="ttml#styling">' +
'<region xml:id="subtitleArea" />' +
'</layout>' +
'<body region="subtitleArea">' +
'<p begin="01:02.05" end="01:02:03.200" style="s1">Test</p>' +
'</body>' +
'</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!

verifyHelper(
[
{start: 62.05, end: 3723.2, text: 'Test', lineAlign: 'center',
align: 'middle', position: 'auto'}
],
'<tt xmlns:tts="ttml#styling">' +
'<styling>' +
'<style xml:id="s1" tts:textAlign="center"/>' +
'</styling>' +
'<layout xmlns:tts="ttml#styling">' +
'<region xml:id="subtitleArea" />' +
'</layout>' +
'<body region="subtitleArea">' +
'<p begin="01:02.05" end="01:02:03.200" style="s1">Test</p>' +
'</body>' +
'</tt>');
});

/**
* @param {!Array} cues
* @param {string} text
Expand All @@ -416,6 +454,8 @@ describe('TtmlTextParser', function() {
expect(result[i].endTime).toBeCloseTo(cues[i].end, 3);
expect(result[i].text).toBe(cues[i].text);

if (cues[i].align)
expect(result[i].align).toBe(cues[i].align);
if (cues[i].lineAlign)
expect(result[i].lineAlign).toBe(cues[i].lineAlign);
if (cues[i].size)
Expand Down