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.

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 v2.0.1 for Toshiba dTV #605

Closed

Conversation

jozefchutka
Copy link
Contributor

Making shaka player working with Toshiba dTV

Mozilla/5.0 (Linux; U; Linux; ja-jp; DTV; TSBNetTV/T3A01C0.0203.0DD) AppleWebKit/536(KHTML, like Gecko) NX/3.0 (DTV; HTML; R1.0;) DTVNetBrowser/2.2 (000039;T3A01C0;0203;0DD) InettvBrowser/2.2 (000039;T3A01C0;0203;0DD) TOSHIBA dTV REGZA BROWSER 43Z700X

Toshiba has unprefixed HTMLMediaElement apis (generateKeyRequest etc.) and events (needkey etc.) while it has prefixed window.performance.webkitNow()

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@jozefchutka
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@jozefchutka jozefchutka changed the title Fix/v2.0.1 toshiba Fix v2.0.1 for Toshiba dTV Nov 24, 2016
@joeyparrish joeyparrish self-assigned this Nov 28, 2016
@joeyparrish joeyparrish added the type: enhancement New feature or request label Nov 28, 2016
@joeyparrish
Copy link
Member

Hi Jozef,

I see a lot of duplication here and I think we can find a better way to solve this. Let me start by clarifying some points.

The platform implements EME v0.1b, but without the webkit prefix. Correct?

The platform implements performance.now with a webkit prefix on the method. Correct?

Thanks,
Joey

@jozefchutka
Copy link
Contributor Author

hi Joey,

  • just like you say, these apis like generateKeyRequest exists without webkit prefix. I cant say its "EME v0.1b" or something else as the available doc for the device doesnt state such information.
  • on the platform window.performance.webkitNow() is available while window.performance.now() is not.
  • also there was a third thing with FakeEvent which I had to replace by CustomEvent see around code event2.initCustomEvent('encrypted'....

The way I worked is I copied PatchedMediaKeysWebkit into PatchedMediaKeys so if you compare these files you will see they are almost identical.

@joeyparrish
Copy link
Member

Yes, the fact that they are almost identical makes me think there should be a solution that avoids all this code duplication. And based on the API, it looks like the very old EME v0.1b API, which is what we polyfill on top of in patchedmediakeys_webkit.js.

I think you should create separate polyfills for each of these things.

For performance.now, it should be very simple to use performance.now = performance.now || performance.webkitNow. You can follow lib/polyfill/fullscreen.js as a template, but your performance polyfill will be much simpler than the fullscreen polyfill.

For EME, let's refactor the existing v0.1b polyfill so that it doesn't hard-code the prefix. If shaka.polyfill.PatchedMediaKeysWebkit.install took a prefix parameter, then the main MediaKeys polyfill could do this:

  if (navigator.requestMediaKeySystemAccess &&
      MediaKeySystemAccess.prototype.getConfiguration) {
    shaka.log.info('Using native EME as-is.');


  } else if (HTMLMediaElement.prototype.webkitGenerateKeyRequest) {
    shaka.log.info('Using webkit-prefixed EME v0.1b');
    shaka.polyfill.PatchedMediaKeysWebkit.install('webkit');

  } else if (HTMLMediaElement.prototype.generateKeyRequest) {
    shaka.log.info('Using unprefixed EME v0.1b');
    shaka.polyfill.PatchedMediaKeysWebkit.install('');


  } else if (window.MSMediaKeys) {
    shaka.log.info('Using ms-prefixed EME v20140218');
    shaka.polyfill.PatchedMediaKeysMs.install();
  } else {
    shaka.log.info('EME not available.');
    shaka.polyfill.PatchedMediaKeysNop.install();
  }

Something in this direction would be acceptable, but I don't want to duplicate the entire file. That will lead to bugs in the long run.

Since we do not have a Toshiba TV for development, would you be willing to work on this and revise your PR?

@joeyparrish joeyparrish added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 29, 2016
@jozefchutka
Copy link
Contributor Author

@joey it makes sense to parametrize the existing PatchedMediaKeys. Do you also have any idea how to handle the FakeEvent problem for 'encrypted' event? I can also try to polyfill performance.now, last time there was some issue with that.
However, I have no idea of the timeline and when the update would be ready, hopefully within next few weeks...

@joeyparrish
Copy link
Member

@jozefchutka, I don't really know what the issue is what FakeEvent. Can you provide some details? What's the error?

@jozefchutka
Copy link
Contributor Author

using FakeEvent it throws UNSPECIFIED_EVENT_TYPE_ERR: DOM Events Exception 0: The Event's type was not specified by initializing the event before the method was called. so I replaced it with CustomEvent (copied from PatchedMediaKeysMs)

@joeyparrish
Copy link
Member

Ah, I see. In onWebkitNeedKey_, we're dispatching FakeEvent on a real EventTarget, as opposed to a FakeEventTarget. That seems like a bug. I think that one instance should definitely be replaced with CustomEvent.

Let us know how it's going refactoring the polyfill, and if we can do anything else to help.

@jozefchutka
Copy link
Contributor Author

@joeyparrish updated. see the latest PR and changes. There are some issues with polyfilling performance, it seems that it only lives for a thread or something, lets say if I do

window.performance.now = window.performance.webkitNow

it works in the next line, but it doesnt work after a second or so...

Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

This looks really good overall. If polyfilling performance.now doesn't work on your platform, then it's really not a big deal. Falling back to Date.now() is good enough. I recommended a few small changes, but otherwise I like this very much. Thank you for contributing!

// Alias.
var PatchedMediaKeysWebkit = shaka.polyfill.PatchedMediaKeysWebkit;
PatchedMediaKeysWebkit.Prefix = prefix;
Copy link
Member

Choose a reason for hiding this comment

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

This should be declared somewhere. It should also be private to the class, which means it must end in an underscore. For example:

/** @Private {string} */
shaka.polyfill.PatchedMediaKeysWebkit.prefix_ = '';

* @param {string} api
* @return {string}
*/
shaka.polyfill.PatchedMediaKeysWebkit.prefixApi = function(api) {
Copy link
Member

Choose a reason for hiding this comment

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

This should also be private.

@@ -702,8 +727,11 @@ shaka.polyfill.PatchedMediaKeysWebkit.MediaKeySession.prototype.generate_ =
// GKR can throw an exception. If this occurs, wait 10 ms and try again at
// most once. This situation should only occur when init data is available
// ahead of the 'needkey' event.

var prefixApi = shaka.polyfill.PatchedMediaKeysWebkit.prefixApi;
var prefixedApi = prefixApi('generateKeyRequest');
Copy link
Member

Choose a reason for hiding this comment

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

Having an alias variable called "prefixApi" and another variable called "prefixedApi" is a little confusing and could easily lead to typos. Please rename this to "generateKeyRequestName", which will both avoid confusion, describe which API it references, and indicate that it is a string, not a function.

@@ -775,8 +803,10 @@ shaka.polyfill.PatchedMediaKeysWebkit.MediaKeySession.prototype.update_ =
keyId = null;
}

var prefixApi = shaka.polyfill.PatchedMediaKeysWebkit.prefixApi;
var prefixedApi = prefixApi('addKey');
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to "addKeyName". See above for rationale.

@@ -852,8 +882,10 @@ shaka.polyfill.PatchedMediaKeysWebkit.MediaKeySession.prototype.close =
// it to clean up resources in v0.1b. We still consider the session closed.
// We can't let the exception propagate because MediaKeySession.close()
// should not throw.
var prefixApi = shaka.polyfill.PatchedMediaKeysWebkit.prefixApi;
var prefixedApi = prefixApi('cancelKeyRequest');
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to "cancelKeyRequestName". See above for rationale.

@joeyparrish joeyparrish removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Dec 13, 2016
@jozefchutka
Copy link
Contributor Author

jozefchutka commented Dec 14, 2016

hi Joey, I did these changes you requested, now it will take some time to test as I do not have access to the device right away, I will confirm its working here asap (today).

@joeyparrish
Copy link
Member

Everything looks good now. I'll run it through the standard checks on our build bot and do some additional testing on Safari.

@shaka-bot
Copy link
Collaborator

Testing in progress...

@joeyparrish
Copy link
Member

Oh, one thing is off, actually. This should be merged into master, not v2.0.x. I will have to merge manually to make that happen. I'll make sure it gets cherry-picked to v2.0.x for our next bugfix release, though.

@shaka-bot
Copy link
Collaborator

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 891, skipped 37

@joeyparrish
Copy link
Member

Those failures are unrelated to your change and have already been fixed. I'm overriding the build bot on this one.

@shaka-bot
Copy link
Collaborator

shaka-bot commented Dec 14, 2016

Test failures overridden.

I've got no strings to hold me down, to make me smile, to make me frown! I've got no strings, as you can see. There are no strings on me!

@joeyparrish
Copy link
Member

All tests are passing on Safari, which is the only browser still using this polyfill. I'm going to go ahead and merge it into master. I will also cherry-pick it to v2.0.x and include it in our upcoming v2.0.2 release.

joeyparrish pushed a commit that referenced this pull request Dec 14, 2016
Toshiba has EME v0.1b, but without the webkit prefix.  This generalizes
the WebKit MediaKeys polyfill to work with or without a prefix.
@joeyparrish
Copy link
Member

Oh, I forgot to wait for you to confirm that it's working well on the Toshiba TV! I got a little ahead of myself.

I'm going to go ahead and close this, since I already merged it. Please let us know the results of your testing when you have a chance. If there is any more work needed to get Toshiba TVs working as expected, I would be happy to review another PR. I apologize if this is an inconvenience.

@jozefchutka
Copy link
Contributor Author

Hi Joey, thanks for merging, latest master works with dTV. There will be one more PR sooner or later as we dont see adaptation switching for widevine video on dTV.... intestigating

@jozefchutka jozefchutka deleted the fix/v2.0.1-toshiba branch December 15, 2016 09:08
ismena pushed a commit that referenced this pull request Dec 15, 2016
Toshiba has EME v0.1b, but without the webkit prefix.  This generalizes
the WebKit MediaKeys polyfill to work with or without a prefix.
@joeyparrish joeyparrish added the platform: TV/STB Issues affecting smart TV or set-top box platforms label Jul 17, 2018
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform: TV/STB Issues affecting smart TV or set-top box platforms status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants