7 args TextTrackCue constructor #643

jozefchutka · 2017-01-04T11:15:33Z

Toshiba dTV seems to have a different set of constructor arguments for TextTrackCue, just like the ones used here
http://www.w3schools.com/TAGs/av_met_addtexttrack.asp . Not sure what are the extra arguments but empty strings and true did the job. This also solves #635

joeyparrish · 2017-01-04T18:07:20Z

I'm sorry, but the Toshiba dTV is not up to spec. If you can detect and fix it with a polyfill, that would be an acceptable solution. But we won't put platform-specific hacks into the main codebase for a user agent that doesn't follow specs.

Try checking TextTrackCue.length. That should allow you to detect the number of arguments it takes and polyfill it to the correct behavior.

jozefchutka · 2017-01-04T20:09:22Z

Do you have any recommendation how to polyfill this? As TextTrackCue exist on window object but requires arguments just like these:
http://www.w3schools.com/TAGs/av_met_addtexttrack.asp
https://www.w3.org/WAI/GL/wiki/Using_the_TextTrack_API_to_provide_captions_dynamically
and I am not able to find other specs that would define official constructor and its arguments.

joeyparrish · 2017-01-04T21:56:43Z

In an earlier draft of the spec, TextTrackCue was all there was. IE11 and Edge only have this 3-argument TextTrackCue. Later, VTTCue was introduced as a concrete implementation, at which point TextTrackCue became an abstract interface. Therefore you will not find a TextTrackCue constructor in the specs now.

I recommend you detect the 7-argument TextTrackCue and implement a standard VTTCue constructor instead. Here's the spec: https://w3c.github.io/webvtt/#the-vttcue-interface

Your implementation of VTTCue can then reorder the arguments and call your 7-arg TextTrackCue. It may not be obvious, but you can return an object from a constructor in JavaScript. For example:

function FakeArray() {
  return new Array();
}
var x = new FakeArray();
console.log('x is Array?', x instanceof Array);

jozefchutka · 2017-01-05T11:29:20Z

TextTrackCue.prototype.constructor.length doesnt return 7 but 1, and so there is no other reliable way of detecting constructor argument count. We will implement polyfill on project level as this (most probably) will not get approved.

joeyparrish · 2017-01-05T20:06:38Z

lib/polyfill/vttcue.js

+ */
+shaka.polyfill.VTTCue.from7ArgsTextTrackCue_ = function(startTime, endTime,
+    text) {
+  return new window['TextTrackCue'](text, startTime, endTime, '', '', '', true);


I assume you're quoting the access here to avoid the compiler complaining about the "wrong" arguments/types. Please add a comment to that effect.

joeyparrish · 2017-01-05T20:16:06Z

lib/polyfill/vttcue.js

+
+/**
+ * Draft spec TextTrackCue with 7 constructor arguments.
+ * See {@link https://goo.gl/CSPvbz Using the TextTrack API} and examples.


This link doesn't specify the 7 arguments or their meaning. It gives an example with 4 arguments, and the examples have the text in argument 4, not argument 1 as you have below.

I tracked down the history as best I could.

Here we had the first appearance of TextTrackCue in draft 20110113 (no constructor).

In draft 20120329, we had the first constructor on TextTrackCue (6 arguments, not 7).

In draft 20121025, the constructor was changed to 3 arguments (as implemented by IE11 and Edge today).

In draft 20121217, we have the final appearance of a TextTrackCue constructor. After this, it became abstract.

Are you sure it's 7 on Toshiba and not 6?

righ I used wrong link in the comment, this one is the one with 7 args http://www.w3schools.com/TAGs/av_met_addtexttrack.asp just like on Toshiba

joeyparrish · 2017-01-05T20:28:34Z

@jozefchutka, let's not give up just yet. You've done all the rest of the work, so I think we can find another way to do the detection. Here are a couple more options:

Parse navigator.userAgent and detect the Toshiba. The down side is that similar platforms, if there are any, would miss out on the polyfill.
Detect TextTrackCue.length == 1. I did some testing, and no currently supported desktop browser looks like this today. Here's a table:

browser	TextTrackCue.length	VTTCue.length
Chrome, Opera, Firefox	0 (abstract, can't be called)	3
Safari	3	3
IE11, Edge	0 (concrete, can still be called)	(VTTCue does not exist)

jozefchutka · 2017-01-06T11:41:21Z

what would you say if I try catch constructing TextTrackCue with 3 and 7 args as a part of polyfill?

joeyparrish · 2017-01-06T22:15:22Z

I would prefer detecting either TextTrackCue.length or navigator.userAgent. Catching exceptions can be expensive, so that should never be part of a success path. If we are parsing a VTT file with thousands of cues, running a loop over each cue in which we catch an exception and try again with different args would really hurt performance.

jozefchutka · 2017-01-09T00:14:14Z

Surely I do not want exceptions during real playback, but only during initial polyfill install phase. Once correct construction is detected, the polyfill would reference the right one.

joeyparrish · 2017-01-09T03:46:52Z

Okay, sounds good. One caught exception at install time is no big deal.

jozefchutka · 2017-01-13T16:07:54Z

hi Joey,
I have updated the PR. As I do not have the device by hand for test, please keep this PR open until confirmed its working. You were right about TextTrackCue.length returning 6 on Toshiba (the tests I was doing using TextTrackCue.prototype.constructor.length were unnecessary/invalid). With this polyfill in place, max one exception will be handled during install phase in potential rare case (IE/EDGE).

joeyparrish · 2017-01-13T19:32:15Z

lib/polyfill/vttcue.js

+  }
+
+  if (!window.TextTrackCue) {
+    shaka.log.info('VTTCue not available.');


This shouldn't happen, since all the browsers we support have either VTTCue or TextTrackCue. So we probably don't need a no-op polyfill for this case.

But if this were to happen, we would throw at new VTTCue in TextEngine, so this message should at least be upgraded to shaka.log.error.

joeyparrish · 2017-01-13T19:32:45Z

lib/polyfill/vttcue.js

+    return;
+  }
+
+  var constructorLength = window.TextTrackCue.length;


You can drop window here now that TextTrackCue is sure to exist.

joeyparrish · 2017-01-13T19:34:48Z

I just had a couple small comments on the code. I'll wait for those revisions, and for you to test on the Toshiba.

It looks really good, and it looks like it cleans things up for non-Toshiba platforms as well. Thanks for contributing!

jozefchutka · 2017-01-17T16:44:38Z

@joeyparrish this is confirmed working on Toshiba, ready to merge from my side

joeyparrish · 2017-01-17T18:24:33Z

Great, I'll test it on the build bot and on all the browsers in our test lab.

shaka-bot · 2017-01-17T18:25:01Z

Testing in progress...

shaka-bot · 2017-01-17T18:33:11Z

Failure:

Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser supports align setting FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:267:5)
  
Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser supports multiple settings FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:277:5)
  
Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser supports timestamps with one-digit hour at start time FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:294:5)
  
Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser supports timestamps with one-digit hour at end time FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:311:5)
  
Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser supports stamps with one-digit hours at start & end time FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:328:5)
  
Chrome_55_0_2883_(Linux_0_0_0).VttTextParser VttTextParser uses relative timestamps if configured to FAILED
  Expected 'middle' to be 'center'.
      at verifyHelper (test/media/vtt_text_parser_unit.js:456:33)
      at Object.<anonymous> (test/media/vtt_text_parser_unit.js:345:5)
  

Failed 6 tests, passed 947, skipped 41

joeyparrish · 2017-01-17T18:36:23Z

There are some tests that modify shaka.media.TextEngine.CueConstructor to simulate various behaviors from the platform. Looks like they will need to be updated to work on VTTCue now that CueConstructor is gone.

joeyparrish · 2017-01-17T18:43:38Z

I have verified that these substitutions in test/media/ttml_text_parser_unit.js and test/media/vtt_text_parser_unit.js fix the issue:

originalCueConstructor => originalVTTCue
shaka.media.TextEngine.CueConstructor => window.VTTCue

I'll make these changes and manually merge. Thanks!

joeyparrish · 2017-01-17T19:00:41Z

Merged as ca299b2.

This adds a VTTCue polyfill for IE/Edge (3-arg TextTrackCue) and Toshiba dTV (6-arg TextTrackCue). Closes #635

7 args TextTrackCue constructor

2bef3dc

jozefchutka mentioned this pull request Jan 4, 2017

TextTrackCue constructor throws on Toshiba dTV #635

Closed

joeyparrish self-assigned this Jan 4, 2017

joeyparrish added the type: enhancement New feature or request label Jan 4, 2017

jozefchutka added 3 commits January 5, 2017 11:21

VTTCue polyfill

a35128c

fixed type typo and added logs

0da3d00

added VTTCue polyfill into shaka-player.uncompiled.js

26ea585

jozefchutka closed this Jan 5, 2017

joeyparrish reviewed Jan 5, 2017

View reviewed changes

joeyparrish reopened this Jan 5, 2017

jozefchutka added 2 commits January 13, 2017 16:56

VTTCue polyfill with 3 and 6 TextTrackCue constructor arguments.

914b285

Merge branch 'master' into fix/TextTrackCue

e43edba

joeyparrish reviewed Jan 13, 2017

View reviewed changes

code cleanup

6e28286

joeyparrish closed this Jan 17, 2017

joeyparrish pushed a commit that referenced this pull request Jan 17, 2017

VTTCue polyfill (#643)

ca299b2

This adds a VTTCue polyfill for IE/Edge (3-arg TextTrackCue) and Toshiba dTV (6-arg TextTrackCue). Closes #635

joeyparrish pushed a commit that referenced this pull request Jan 19, 2017

VTTCue polyfill (#643)

05d17ca

This adds a VTTCue polyfill for IE/Edge (3-arg TextTrackCue) and Toshiba dTV (6-arg TextTrackCue). Closes #635

joeyparrish added the platform: TV/STB Issues affecting smart TV or set-top box platforms label Jul 17, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7 args TextTrackCue constructor #643

7 args TextTrackCue constructor #643

jozefchutka commented Jan 4, 2017 •

edited

Loading

joeyparrish commented Jan 4, 2017

jozefchutka commented Jan 4, 2017

joeyparrish commented Jan 4, 2017

jozefchutka commented Jan 5, 2017 •

edited

Loading

joeyparrish Jan 5, 2017

joeyparrish Jan 5, 2017

jozefchutka Jan 6, 2017

joeyparrish commented Jan 5, 2017

jozefchutka commented Jan 6, 2017

joeyparrish commented Jan 6, 2017

jozefchutka commented Jan 9, 2017

joeyparrish commented Jan 9, 2017

jozefchutka commented Jan 13, 2017 •

edited

Loading

joeyparrish Jan 13, 2017

joeyparrish Jan 13, 2017

joeyparrish commented Jan 13, 2017

jozefchutka commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

shaka-bot commented Jan 17, 2017

shaka-bot commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

7 args TextTrackCue constructor #643

7 args TextTrackCue constructor #643

Conversation

jozefchutka commented Jan 4, 2017 • edited Loading

joeyparrish commented Jan 4, 2017

jozefchutka commented Jan 4, 2017

joeyparrish commented Jan 4, 2017

jozefchutka commented Jan 5, 2017 • edited Loading

joeyparrish Jan 5, 2017

Choose a reason for hiding this comment

joeyparrish Jan 5, 2017

Choose a reason for hiding this comment

jozefchutka Jan 6, 2017

Choose a reason for hiding this comment

joeyparrish commented Jan 5, 2017

jozefchutka commented Jan 6, 2017

joeyparrish commented Jan 6, 2017

jozefchutka commented Jan 9, 2017

joeyparrish commented Jan 9, 2017

jozefchutka commented Jan 13, 2017 • edited Loading

joeyparrish Jan 13, 2017

Choose a reason for hiding this comment

joeyparrish Jan 13, 2017

Choose a reason for hiding this comment

joeyparrish commented Jan 13, 2017

jozefchutka commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

shaka-bot commented Jan 17, 2017

shaka-bot commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

joeyparrish commented Jan 17, 2017

jozefchutka commented Jan 4, 2017 •

edited

Loading

jozefchutka commented Jan 5, 2017 •

edited

Loading

jozefchutka commented Jan 13, 2017 •

edited

Loading