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

glhvta · 2020-03-04T18:27:27Z

Scope of PR:

parse ttp:cellResolution and provide it to shaka.text.Cue, so text displayers can use it in next calculations
add support of fontSize in '%' and 'c' to ttml_text_parser
add support of font size in cell resolution units to shaka.ui.TextDisplayer
add unit tests

Note:

add partial support of font size in percentages to shaka.ui.TextDisplayer

*From w3c spec:
Percentages: if not region element, then relative to the parent element's computed font size; otherwise, relative to the Computed Cell Size.
Currently will be calculated relative to the Computed Cell Size.

Spec:
tts:fontSize - https://www.w3.org/TR/ttml1/#style-attribute-fontSize
ttp:cellResolution - https://www.w3.org/TR/ttml1/#parameter-attribute-cellResolution
- https://www.w3.org/TR/ttml1/#style-value-length

Useful links that helped during development and documentation investigation:
https://www.w3.org/TR/ttml1/
http://sandflow.com/imsc1_1/ - IMSC1 Renderer
https://trac.videolan.org/vlc/ticket/18347

Sample TTML that can be used for testing:
ttml-captions-test.zip

Closes #2403

joeyparrish · 2020-03-04T18:59:26Z

lib/text/cue.js

+ * @const
+ * @export
+ */
+shaka.text.Cue.cellResolution = {


The enums below and above are possible options for each field. This is a set of defaults. I would prefer you move it to the initialization of the member:

this.cellResolution = [32, 15]; // columns, rows

joeyparrish · 2020-03-04T19:01:05Z

lib/text/ttml_text_parser.js

+    // If not specified, the number of columns and rows must be considered
+    // to be 32 and 15 respectively


You've already defined a default in the Cue constructor. Why duplicate it?

agree,
changed the logic, so now cue.cellResolution default value will be replaced only if ttp:cellResolution was presented in ttml

joeyparrish · 2020-03-04T19:03:25Z

test/text/ttml_text_parser_unit.js

+            cellResolution: [
+              Cue.cellResolution.COLUMNS,
+              Cue.cellResolution.ROWS,
+            ],


If it doesn't matter what the cellResolution is in this output cue (since it's not using c units), you can omit this field. verifyHelper below uses jasmine.objectContaining() to match, so any field you don't specify in the expectation won't count.

In fact, it seems that this test case doesn't have much to do with cellResolution at all, so maybe you could omit it.

this test was added to check that Cue has default cellResolution when ttp:cellResolution was not presented in ttml
I guess it would be nice to leave this check

joeyparrish · 2020-03-04T19:05:13Z

test/ui/text_displayer_unit.js

@@ -39,15 +41,20 @@ describe('UITextDisplayer', () => {
  }


-  beforeAll(() => {
+  beforeEach(() => {


The addition of video elements to the DOM should be beforeAll instead of beforeEach.

You could move textDisplayer setup specifically to beforeEach and leave the rest in beforeAll.

yes, you are right, thanks

was fixed

joeyparrish · 2020-03-04T19:11:23Z

test/ui/text_displayer_unit.js

+        jasmine.objectContaining({'font-size': expectedFontSize}));
+  });
+
+  it('correctly displays styles for percentages units', async () => {


Thank you for adding these tests!

joeyparrish · 2020-03-04T19:12:01Z

ui/text_displayer.js

@@ -268,11 +268,15 @@ shaka.ui.TextDisplayer = class {
      captionsStyle.margin = '0';
    }

+    const {convertLengthValue_} = shaka.ui.TextDisplayer;


You only call this once. Just refer to it directly with its full name at the call site.

joeyparrish · 2020-03-04T19:12:53Z

ui/text_displayer.js

+  }
+
+  /**
+   * Converts length value


"Converts length value" returning "string" is not enough information for the reader. What format is the string? What sort of conversion?

the description was updated,
could you please take a look if I need to add more details

joeyparrish · 2020-03-04T19:13:18Z

ui/text_displayer.js

+  static getLengthValueInfo_(lengthValue) {
+    const matches = new RegExp(/(\d*\.?\d+)([a-z]+|%+)/).exec(lengthValue);
+
+    if (!matches ) {


style nit: remove extra space inside parens

joeyparrish · 2020-03-04T19:21:55Z

ui/text_displayer.js

+   *
+   * @private
+   * */
+  static getComputedValue_(value, cue, videoContainer) {


"computed value" is a bit vague. This method actually does something very specific: computes an absolute value in pixels based on cell and video size.

was renamed to getAbsoluteLengthInPixels_ and added the description for this method,
could you please take a look if I need to add more details
thanks

externs/shaka/text.js

glhvta · 2020-03-05T10:22:21Z

@theodab @joeyparrish , thank you for the review and quick feedback!

The comments were fixed. Could you please take a look and let me know if there is anything else that needs to be done.

shaka-bot · 2020-03-05T19:25:38Z

All tests passed!

theodab · 2020-03-05T20:36:03Z

Whoops, I accidentally started an extraneous test run. Sorry about that.

joeyparrish

Looks great overall. Just a few things I'd like you to change. Thanks!

joeyparrish · 2020-03-05T21:12:05Z

externs/shaka/text.js

+     * The number of horizontal and vertical cells into which
+     * the Root Container Region area is divided
+     *
+     * @typedef {{ columns: number, rows: number }}


This looked different from what we would normally use, so I checked. It seems that with this annotation, the compiler doesn't enforce the type correctly. What you want is @type {{ columns: number, rows: number }} instead.

A typedef in Closure Compiler is used for creating a new type, but not for assigning a type to a member. I know the compiler can be arcane, so I'm sorry for the trouble.

thanks for the explanation!
done

joeyparrish · 2020-03-05T21:14:18Z

lib/text/ttml_text_parser.js

@@ -239,12 +245,13 @@ shaka.text.TtmlTextParser = class {
   * @param {!Array.<!shaka.text.CueRegion>} cueRegions
   * @param {boolean} whitespaceTrim
   * @param {boolean} isNested
+   * @param {?Object} cellResolution


"Object" is too general a type. Please use a record type like you did in Cue:
{?{columns: number, rows: number}}, so that the compiler can catch a caller using the wrong fields.

joeyparrish · 2020-03-05T21:15:35Z

lib/text/ttml_text_parser.js

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

+    if (cellResolution) {
+      cue.cellResolution = Object.assign({}, cellResolution);


You don't need Object.assign. The provided value won't be changed by the method that generated it, and you'll preserve type info in the compiler by assigning it directly.

removed Object.assign

joeyparrish · 2020-03-05T21:17:41Z

lib/text/ttml_text_parser.js

+   * @private
+   */
+  static getCellResolution_(cellResolution) {
+    const matches = /^(\d+) (\d+)$/.exec(cellResolution);


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.

done,
added check to getCellResolution_ method

joeyparrish · 2020-03-05T21:19:49Z

test/ui/text_displayer_unit.js

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

+  afterEach(() => {
+    textDisplayer.destroy();


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

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

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.

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

done

joeyparrish · 2020-03-05T21:24:26Z

ui/text_displayer.js

+  */
+  static convertLengthValue_(lengthValue, cue, videoContainer) {
+    const lengthValueInfo =
+      shaka.ui.TextDisplayer.getLengthValueInfo_(lengthValue);


Style nit: indent +2 spaces

sorry, not fully understood
should I remove 2 spaces to be like this

const lengthValueInfo = shaka.ui.TextDisplayer.getLengthValueInfo_(lengthValue);

Sorry, I meant +2 more, for a total of 4 more than the previous line

joeyparrish · 2020-03-05T21:24:40Z

ui/text_displayer.js

+      case '%':
+        return shaka.ui.TextDisplayer.getAbsoluteLengthInPixels_(
+            value / 100, cue, videoContainer
+        );


style nit: move to previous line

joeyparrish · 2020-03-05T21:24:45Z

ui/text_displayer.js

+      case 'c':
+        return shaka.ui.TextDisplayer.getAbsoluteLengthInPixels_(
+            value, cue, videoContainer
+        );


style nit: move to previous line

joeyparrish

Looks good to me! Thanks for all your hard work.

shaka-bot · 2020-03-06T16:51:17Z

All tests passed!

joeyparrish · 2020-03-18T23:25:14Z

Unfortunately, this change did not cherry-pick cleanly to v2.5.x, and so will not be in an official release until v2.6.0.

glhvta and others added 7 commits March 4, 2020 01:51

Add support of cellResolution fontSize

baab7c1

Update unit tests for parsing cellResolution from ttml cue

a8d5c64

Add unit test in text_displayer for cellResolution support

40ee5d7

Add support of percentages values of fontSize

1306ca5

Update authors and contributors

7e857b6

Change cellResolution type in Cue

27f429b

Rename variable in getLengthValueInfo_ method

7757a3d

joeyparrish self-assigned this Mar 4, 2020

joeyparrish added the type: enhancement New feature or request label Mar 4, 2020

joeyparrish requested changes Mar 4, 2020

View reviewed changes

glhvta and others added 2 commits March 5, 2020 00:11

Fix review remarks

7514688

Return blank line

12e6842

glhvta requested a review from joeyparrish March 4, 2020 21:36

theodab reviewed Mar 4, 2020

View reviewed changes

externs/shaka/text.js Show resolved Hide resolved

Convert cellResolution from array to object

79fae68

glhvta requested a review from theodab March 5, 2020 10:22

avelad mentioned this pull request Mar 5, 2020

Add ebutts:linePadding to ttml parser #2443

Merged

glhvta added 2 commits March 5, 2020 13:56

correct fontSize value in test

d96aa9f

Refer to methods directly

336b720

shaka-project deleted a comment from shaka-bot Mar 5, 2020

joeyparrish requested changes Mar 5, 2020

View reviewed changes

Fix review remarks

f47f286

glhvta requested a review from joeyparrish March 5, 2020 22:22

Pass async callback to afterEach

93df379

joeyparrish approved these changes Mar 6, 2020

View reviewed changes

joeyparrish merged commit d6de4e7 into shaka-project:master Mar 6, 2020

glhvta deleted the add-cell-resolution-support branch March 6, 2020 16:56

github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023

		// If not specified, the number of columns and rows must be considered
		// to be 32 and 15 respectively

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

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

Conversation

glhvta commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glhvta Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glhvta Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glhvta Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

glhvta commented Mar 5, 2020

shaka-bot commented Mar 5, 2020

theodab commented Mar 5, 2020

joeyparrish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyparrish left a comment

Choose a reason for hiding this comment

shaka-bot commented Mar 6, 2020

joeyparrish commented Mar 18, 2020

glhvta Mar 4, 2020 •

edited

Loading

glhvta Mar 4, 2020 •

edited

Loading

glhvta Mar 4, 2020 •

edited

Loading