Add new lines in TTML cues with nested tags by sanbornhilland · Pull Request #584 · shaka-project/shaka-player

sanbornhilland · 2016-11-11T23:06:53Z

This modifies the fix from #572. We have streams that also have   tags nested in  tags that were getting missed by #572

ismena · 2016-11-12T00:05:50Z

lib/media/ttml_text_parser.js

 };


+/**


Let's rephrase to something like "Replaces
tags with /m characters"

Strictly speaking it's not actually replacing tags. How about "Insert '\n' characters into
tags"

Current comment looks good to me. Seems accurate.

ismena · 2016-11-12T00:06:51Z

lib/media/ttml_text_parser.js


+/**
+ * Insert \n where <br> tags are found
+ *


{!Element} for consistency (instead of Node)

I had that but it failed.

The build failed? Ok, please leave a TODO above the param declaration (line 212) that having param Element here gives build problems and I'll look at it when we merge the PR.

At the call site, the type of cueElement is !Element, so using the same here should work. What was the error?

Just re ran the build and looked more closely. The issue is that because this function calls itself to drill down and find nested tags, the nested nodes are not guaranteed to be elements. Initial call is always !Element but subsequent calls are not guaranteed.

/Users/sanbornh/Documents/shaka-player/lib/media/ttml_text_parser.js:223: ERROR - actual parameter 1 of shaka.media.TtmlTextParser.addNewLines_ does not match formal parameter found : Node required: Element shaka.media.TtmlTextParser.addNewLines_(childNodes[i]);

We solved this elsewhere in with an assertion and a cast:

if (childNodes[i].nodeType == Node.ELEMENT_NODE && ...) { goog.asserts.assert(childNodes[i] instanceof Element, 'Node should be Element!'); var leafChildren = shaka.media.TtmlTextParser.getLeafNodes_( /** @type {Element} */(childNodes[i]));

I'm not familiar with the problem we are attempting to solve here. Can you describe why it is important to be using elements instead of nodes?

There's no runtime difference. It's just a matter of describing the types accurately and asserting to the compiler that it is that type.

Element is a subclass of Node, but childNodes contains all Nodes, even those that are not Elements. If we only want to walk through the Elements, we could use children, but some older browsers won't support that. Instead we use childNodes, and we have to both filter out the Elements by nodeType, and we have to tell the compiler that this is a safe conversion.

Okay, thanks, I appreciate the explanation. Will update.

No problem. I'm sorry that the compiler is a bit of a burden in this instance. It's not perfect by any means, but I think on balance, it has helped to catch and prevent a lot of bugs.

ismena · 2016-11-12T00:12:41Z

lib/media/ttml_text_parser.js

+
+  for (var i = 0; i < childNodes.length; i++) {
+    if (childNodes[i].nodeName == 'br' && i > 0) {
+      childNodes[i - 1].textContent += '\n';


Any advantages to "!==" operator over "childNodes.length > 0" here?

Not that I can see. Happy to change it if people prefer it that way.

Yup, please change.

ismena · 2016-11-12T00:15:17Z

test/media/ttml_text_parser_unit.js

        'end="01:02:03.200">Line1<br/>Line2</p></body></tt>');
  });

+  it('inserts a newline on br in elements nested in cue', function() {


Let's merge this test and a previous one into a single "it('replaces
tags with newline characters')" with 2 calls to verifyHelper in it.
(Like in 'disregards empty divs and ps' above which has 3 calls).

joeyparrish · 2016-12-06T21:37:10Z

The only part of this that I still wanted changed was the use of Element vs Node, but it's not that important. I'm going to run this PR through the build bot and merge it if everything checks out.

shaka-bot · 2016-12-06T21:37:39Z

Testing in progress...

shaka-bot · 2016-12-06T21:55:24Z

Failure:

Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine VOD plays FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine VOD plays at high playback rates FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine VOD can handle buffered seeks FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine VOD can handle unbuffered seeks FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine Live plays through Period transition FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine Live can handle seeks ahead of availability window FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  
Chrome_55_0_2883_(Linux_0_0_0).StreamingEngine StreamingEngine Live can handle seeks behind availability window FAILED
  Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  

Failed 7 tests, passed 902, skipped 37

joeyparrish · 2016-12-06T22:36:27Z

I think those errors have already been fixed in master (#623). I will cherry-pick this and test it rebased on top of master.

shaka-bot · 2016-12-06T22:53:56Z

Whoops! Trying again with a rebase...

shaka-bot · 2016-12-06T22:56:59Z

Sorry about that. Looks good! All tests passed after rebasing.

This modifies the fix from #572. Streams that also have tags nested in tags were getting missed by #572.

Add new lines in TTML cues with nested tags

8388f74

ismena reviewed Nov 12, 2016

View reviewed changes

Sanborn Hilland added 2 commits November 16, 2016 13:53

Combine ttml text parser newline tests into one

425e7c0

Refactor ttml text parser conditional statement

8ecfe7e

joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 29, 2016

joeyparrish removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Dec 6, 2016

joeyparrish merged commit 71ce370 into shaka-project:master Dec 6, 2016

ismena pushed a commit that referenced this pull request Dec 15, 2016

Add new lines in TTML cues with nested tags (#584)

396c5f6

This modifies the fix from #572. Streams that also have tags nested in tags were getting missed by #572.

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

Conversation

sanbornhilland commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joeyparrish commented Dec 6, 2016

Uh oh!

shaka-bot commented Dec 6, 2016

Uh oh!

shaka-bot commented Dec 6, 2016

Uh oh!

joeyparrish commented Dec 6, 2016

Uh oh!

shaka-bot commented Dec 6, 2016

Uh oh!

shaka-bot commented Dec 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants