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

Add support of fontSize in percentages and cell resolution unit for TTML captions #2442

Merged
merged 14 commits into from
Mar 6, 2020
Merged

Add support of fontSize in percentages and cell resolution unit for TTML captions #2442

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
baab7c1
Add support of cellResolution fontSize
glhvta Mar 3, 2020
a8d5c64
Update unit tests for parsing cellResolution from ttml cue
glhvta Mar 4, 2020
40ee5d7
Add unit test in text_displayer for cellResolution support
glhvta Mar 4, 2020
1306ca5
Add support of percentages values of fontSize
glhvta Mar 4, 2020
7e857b6
Update authors and contributors
glhvta Mar 4, 2020
27f429b
Change cellResolution type in Cue
glhvta Mar 4, 2020
7757a3d
Rename variable in getLengthValueInfo_ method
glhvta Mar 4, 2020
7514688
Fix review remarks
glhvta Mar 4, 2020
12e6842
Return blank line
glhvta Mar 4, 2020
79fae68
Convert cellResolution from array to object
glhvta Mar 5, 2020
d96aa9f
correct fontSize value in test
glhvta Mar 5, 2020
336b720
Refer to methods directly
glhvta Mar 5, 2020
f47f286
Fix review remarks
glhvta Mar 5, 2020
93df379
Pass async callback to afterEach
glhvta Mar 6, 2020
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
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ SameGoal Inc. <*@samegoal.com>
Sanborn Hilland <sanbornh@rogers.com>
Sander Saares <sander@saares.eu>
TalkTalk Plc <*@talktalkplc.com>
Tatsiana Gelahova <tatsiana.gelahova@gmail.com>
Tomas Tichy <mr.tichyt@gmail.com>
Tomohiro Matsuzawa <thmatuza75@hotmail.com>
Toshihiro Suzuki <t.suzuki326@gmail.com>
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ Sandra Lokshina <ismena@google.com>
Satheesh Velmurugan <satheesh@philo.com>
Semih Gokceoglu <semih.gkcoglu@gmail.com>
Seth Madison <seth@philo.com>
Tatsiana Gelahova <tatsiana.gelahova@gmail.com>
Theodore Abshire <theodab@google.com>
Thomas Stephens <thomas@ustudio.com>
Tim Plummer <objelisks@google.com>
Expand Down
10 changes: 10 additions & 0 deletions externs/shaka/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,16 @@ shaka.extern.Cue = class {
*/
this.backgroundColor;

/**
* The number of horizontal and vertical cells into which
* the Root Container Region area is divided
* cellResolution[0] - columns
* cellResolution[1] - rows
* @type {!Array.<number>}
* @exportDoc
*/
this.cellResolution;

/**
* Image background represented by any string that would be
* accepted in image HTML element.
Expand Down
6 changes: 6 additions & 0 deletions lib/text/cue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,12 @@ shaka.text.Cue = class {
* @exportInterface
*/
this.spacer = false;

/**
* @override
* @exportInterface
*/
this.cellResolution = [32, 15]; // columns, rows
}
};

Expand Down
59 changes: 53 additions & 6 deletions lib/text/ttml_text_parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ shaka.text.TtmlTextParser = class {
let tickRate = null;
let spaceStyle = null;
let extent = null;
let cellResolution = null;
const tts = xml.getElementsByTagName('tt');
const tt = tts[0];
// TTML should always have tt element.
Expand All @@ -91,6 +92,7 @@ shaka.text.TtmlTextParser = class {
frameRateMultiplier =
XmlUtils.getAttributeNS(tt, ttpNs, 'frameRateMultiplier');
tickRate = XmlUtils.getAttributeNS(tt, ttpNs, 'tickRate');
cellResolution = XmlUtils.getAttributeNS(tt, ttpNs, 'cellResolution');
spaceStyle = tt.getAttribute('xml:space') || 'default';
extent = tt.getAttribute('tts:extent');
}
Expand All @@ -107,6 +109,9 @@ shaka.text.TtmlTextParser = class {
const rateInfo = new TtmlTextParser.RateInfo_(
frameRate, subFrameRate, frameRateMultiplier, tickRate);

const cellResolutionInfo =
TtmlTextParser.getCellResolution_(cellResolution);

const metadataElements = TtmlTextParser.getLeafNodes_(
tt.getElementsByTagName('metadata')[0]);
const styles = TtmlTextParser.getLeafNodes_(
Expand All @@ -127,7 +132,8 @@ shaka.text.TtmlTextParser = class {
for (const node of textNodes) {
const cue = TtmlTextParser.parseCue_(
node, time.periodStart, rateInfo, metadataElements, styles,
regionElements, cueRegions, whitespaceTrim, false);
regionElements, cueRegions, whitespaceTrim, false,
cellResolutionInfo);
if (cue) {
ret.push(cue);
}
Expand Down Expand Up @@ -239,12 +245,13 @@ shaka.text.TtmlTextParser = class {
* @param {!Array.<!shaka.text.CueRegion>} cueRegions
* @param {boolean} whitespaceTrim
* @param {boolean} isNested
* @param {?Array.<number>} cellResolution
* @return {shaka.text.Cue}
* @private
*/
static parseCue_(
cueElement, offset, rateInfo, metadataElements, styles, regionElements,
cueRegions, whitespaceTrim, isNested) {
cueRegions, whitespaceTrim, isNested, cellResolution) {
if (isNested && cueElement.nodeName == 'br') {
const cue = new shaka.text.Cue(0, 0, '');
cue.spacer = true;
Expand Down Expand Up @@ -322,7 +329,8 @@ shaka.text.TtmlTextParser = class {
regionElements,
cueRegions,
whitespaceTrim,
/* isNested= */ true
/* isNested= */ true,
cellResolution,
);

if (nestedCue) {
Expand All @@ -334,6 +342,10 @@ shaka.text.TtmlTextParser = class {
const cue = new shaka.text.Cue(start, end, payload);
cue.nestedCues = nestedCues;

if (cellResolution) {
cue.cellResolution = cellResolution;
}

// Get other properties if available.
const regionElement = shaka.text.TtmlTextParser.getElementsFromCollection_(
cueElement, 'region', regionElements, /* prefix= */ '')[0];
Expand Down Expand Up @@ -543,7 +555,12 @@ shaka.text.TtmlTextParser = class {

const fontSize = TtmlTextParser.getStyleAttribute_(
cueElement, region, styles, 'fontSize');
if (fontSize && fontSize.match(TtmlTextParser.unitValues_)) {

const isValidFontSizeUnit = fontSize
&& (fontSize.match(TtmlTextParser.unitValues_)
|| fontSize.match(TtmlTextParser.percentValue_));

if (isValidFontSizeUnit) {
cue.fontSize = fontSize;
}

Expand Down Expand Up @@ -933,6 +950,29 @@ shaka.text.TtmlTextParser = class {

return (milliseconds / 1000) + seconds + (minutes * 60) + (hours * 3600);
}

/**
* If ttp:cellResolution provided returns array
* with cellResolution info, where
* cellResolution[0] - columns
* cellResolution[1] - rows
*
* @param {?string} cellResolution
* @return {?Array.<number>}
* @private
*/
static getCellResolution_(cellResolution) {
const matches = /^(\d+) (\d+)$/.exec(cellResolution);
Copy link
Member

Choose a reason for hiding this comment

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

cellResolution can be null, and this would convert null into the string "null".

While that shouldn't cause any false positives, it may be best to handle !cellResolution explicitly, either here or when the attribute is extracted from the TTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done,
added check to getCellResolution_ method


if (!matches) {
return null;
}

const columns = parseInt(matches[1], 10);
const rows = parseInt(matches[2], 10);

return [columns, rows];
}
};

/**
Expand Down Expand Up @@ -996,9 +1036,16 @@ shaka.text.TtmlTextParser.percentValues_ =
/**
* @const
* @private {!RegExp}
* @example 100px
* @example 0.6% 90%
*/
shaka.text.TtmlTextParser.percentValue_ = /^(\d{1,2}(?:\.\d+)?|100)%$/;

/**
* @const
* @private {!RegExp}
* @example 100px, 8em, 0.80c
*/
shaka.text.TtmlTextParser.unitValues_ = /^(\d+px|\d+em)$/;
shaka.text.TtmlTextParser.unitValues_ = /^(\d+px|\d+em|\d*\.?\d+c)$/;

/**
* @const
Expand Down
61 changes: 57 additions & 4 deletions test/text/ttml_text_parser_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,8 +792,8 @@ describe('TtmlTextParser', () => {
verifyHelper(
[
{
startTime: 62.05,
endTime: 3723.2,
startTime: 1,
endTime: 2,
payload: 'Test',
color: 'red',
backgroundColor: 'blue',
Expand All @@ -803,6 +803,12 @@ describe('TtmlTextParser', () => {
lineHeight: '20px',
fontSize: '10em',
},
{
startTime: 2,
endTime: 4,
payload: 'Test 2',
fontSize: '0.80c',
},
],
'<tt xmlns:tts="http://www.w3.org/ns/ttml#styling">' +
'<styling>' +
Expand All @@ -813,12 +819,14 @@ describe('TtmlTextParser', () => {
'tts:fontStyle="italic" ' +
'tts:lineHeight="20px" ' +
'tts:fontSize="10em"/>' +
'<style xml:id="s2" tts:fontSize="0.80c" />' +
'</styling>' +
'<layout>' +
'<region xml:id="subtitleArea" />' +
'</layout>' +
'<body region="subtitleArea">' +
'<p begin="01:02.05" end="01:02:03.200" style="s1">Test</p>' +
'<p begin="00:01.00" end="00:02.00" style="s1">Test</p>' +
'<p begin="00:02.00" end="00:04.00" style="s2">Test 2</p>' +
'</body>' +
'</tt>',
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
Expand Down Expand Up @@ -875,6 +883,52 @@ describe('TtmlTextParser', () => {
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('cues should have default cellResolution', () => {
verifyHelper(
[
{
startTime: 1,
endTime: 2,
cellResolution: [32, 15],
fontSize: '0.45c',
},
],
'<tt xmlns:tts="http://www.w3.org/ns/ttml#styling">' +
'<styling>' +
'<style xml:id="s1" tts:fontSize="0.45c"/>' +
'</styling>' +
'<body >' +
'<p begin="00:01.00" end="00:02.00" style="s1">Test</p>' +
'</body>' +
'</tt>',
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('parses cellResolution', () => {
verifyHelper(
[
{
startTime: 1,
endTime: 2,
payload: 'Test',
cellResolution: [60, 20],
fontSize: '67%',
},
],
'<tt ' +
'xmlns:ttp="http://www.w3.org/ns/ttml#parameter" ' +
'xmlns:tts="http://www.w3.org/ns/ttml#styling" ' +
'ttp:cellResolution="60 20">' +
'<styling>' +
'<style xml:id="s1" tts:fontSize="67%"/>' +
'</styling>' +
'<body >' +
'<p begin="00:01.00" end="00:02.00" style="s1">Test</p>' +
'</body>' +
'</tt>',
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('chooses style on element over style on region', () => {
verifyHelper(
[
Expand Down Expand Up @@ -913,7 +967,6 @@ describe('TtmlTextParser', () => {
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});


/**
* @param {!Array} cues
* @param {string} text
Expand Down
56 changes: 56 additions & 0 deletions test/ui/text_displayer_unit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ describe('UITextDisplayer', () => {
let video;
/** @type {shaka.ui.TextDisplayer} */
let textDisplayer;
/** @type {number} */
const videoContainerHeight = 450;

/**
* Transform a cssText to an object.
Expand Down Expand Up @@ -42,12 +44,20 @@ describe('UITextDisplayer', () => {
beforeAll(() => {
videoContainer =
/** @type {!HTMLElement} */ (document.createElement('div'));
videoContainer.style.height = `${videoContainerHeight}px`;
document.body.appendChild(videoContainer);
video = shaka.test.UiUtils.createVideoElement();
videoContainer.appendChild(video);
});

beforeEach(() => {
textDisplayer = new shaka.ui.TextDisplayer(video, videoContainer);
});

afterEach(() => {
textDisplayer.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

destroy() is async, so please make this callback async and await the results of destroy().

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, I can miss something, but as I see TextDisplayer destroy method is not async
https://github.com/google/shaka-player/blob/ea0131dc80483c7df2c9ec5c9ed62fbe560f37c3/ui/text_displayer.js#L67-L84

Copy link
Member

Choose a reason for hiding this comment

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

That implementation is not async today, but the underlying interfaces shaka.extern.TextDisplayer and shaka.util.IDestroyable are async and return a Promise. In our codebase, any destroy() is async, and any release() is immediate.

It would be possible for us to add async steps to destroy() in shaka.ui.TextDisplayer at any time, and the tests should honor the contract of that API and use await.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for such a detailed explanation, now I've got it

done

});

it('correctly displays styles for cues', async () => {
/** @type {!shaka.text.Cue} */
const cue = new shaka.text.Cue(0, 100, 'Captain\'s log.');
Expand Down Expand Up @@ -129,4 +139,50 @@ describe('UITextDisplayer', () => {
// 'writing-mode': 'horizontal-tb',
}));
});

it('correctly displays styles for cellResolution units', async () => {
/** @type {!shaka.text.Cue} */
const cue = new shaka.text.Cue(0, 100, 'Captain\'s log.');
cue.fontSize = '0.80c';
cue.cellResolution = [60, 20];

textDisplayer.setTextVisibility(true);
textDisplayer.append([cue]);
// Wait until updateCaptions_() gets called.
await shaka.test.Util.delay(0.5);

// Expected value is calculated based on ttp:cellResolution="60 20"
// videoContainerHeight=450px and tts:fontSize="0.80c" on the default style.
const expectedFontSize = '18px';

const textContainer =
videoContainer.querySelector('.shaka-text-container');
const captions = textContainer.querySelector('span');
const cssObj = parseCssText(captions.style.cssText);
expect(cssObj).toEqual(
jasmine.objectContaining({'font-size': expectedFontSize}));
});

it('correctly displays styles for percentages units', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for adding these tests!

/** @type {!shaka.text.Cue} */
const cue = new shaka.text.Cue(0, 100, 'Captain\'s log.');
cue.fontSize = '0.90c';
cue.cellResolution = [32, 15];

textDisplayer.setTextVisibility(true);
textDisplayer.append([cue]);
// Wait until updateCaptions_() gets called.
await shaka.test.Util.delay(0.5);

// Expected value is calculated based on ttp:cellResolution="32 15"
// videoContainerHeight=450px and tts:fontSize="90%" on the default style.
const expectedFontSize = '27px';

const textContainer =
videoContainer.querySelector('.shaka-text-container');
const captions = textContainer.querySelector('span');
const cssObj = parseCssText(captions.style.cssText);
expect(cssObj).toEqual(
jasmine.objectContaining({'font-size': expectedFontSize}));
});
});
Loading