Feat/ui custom castproxy #2464

tecteun · 2020-03-20T18:00:38Z

This is an attempt at extending ui/ui.js with an optional external castProxy class.

there are still some open ends, but to start the discussion I started this pull request.

If there are any good idea's on how this should better be implemented, I love to hear them!


        Enable custom castproxy in shaka.ui.Overlay


        Allow preventing default for container click / touch

googlebot · 2020-03-20T18:00:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

joeyparrish · 2020-03-20T19:05:42Z

I have some technical feedback around what looks like some compiler issues, but first let's discuss your goals with this change.

What are the main goals of a custom CastProxy?

tecteun · 2020-03-20T20:57:14Z

I have a project where I would like to use the stock ui, with some minor modifications. One of the modifications being the Chromecast sender implementation.
The main goal for this custom CastProxy is to be able to use a different Chromecast receiver implementation, using the default Shaka build.

I now can use a custom CastProxy sender implementation, using CAF v3 api's. This works fine for me, using this additional argument for shaka.ui.Overlay. Though, I'm struggling a bit on how to make closure compiler cope with this externally defined castproxy, and make it a good general addition to the project.

joeyparrish · 2020-03-20T22:39:51Z

Yeah, the compiler is difficult sometimes. I'll leave you some specific technical feedback on the changes about the compiler. Hopefully we'll be able to help you resolve that.

I'm open to this change generally, but I wonder if you'd be willing to share your CAF v3 implementation with us as well. Does it use CAF on the sender side instead of the older Cast SDK? Does it still expect Shaka Player on the receiver side, or does it use the CAF default receiver?

joeyparrish · 2020-03-20T22:44:51Z

externs/shaka/cast.js

@@ -0,0 +1,56 @@
+


When you define externs, you need to have a block comment containing @externs at the top. Please copy the copyright header and @externs block comment from another extern file.

Without that, this file will not be seen as an "external" interface from the environment, and pieces of the implementation can still be renamed and inlined. I see you are using ['foo'] style to avoid renaming, but inlining can't be defeated in this way.

When you fix the externs header, you should see those issues go away.

joeyparrish · 2020-03-20T22:44:51Z

ui/cast_button.js

@@ -39,7 +39,7 @@ shaka.ui.CastButton = class extends shaka.ui.Element {
  constructor(parent, controls) {
    super(parent, controls);

-    /** @private {shaka.cast.CastProxy} */
+    /** @private {shaka.cast.CastProxy|shaka.extern.CastProxy} */


Since shaka.cast.CastProxy is an implementation of shaka.extern.CastProxy, you should just use the base type here.

joeyparrish · 2020-03-20T22:44:51Z

ui/cast_button.js

@@ -94,12 +94,12 @@ shaka.ui.CastButton = class extends shaka.ui.Element {

    /** @private */
  async onCastClick_() {
-    if (this.castProxy_.isCasting()) {
-      this.castProxy_.suggestDisconnect();
+    if (this.castProxy_['isCasting']()) {


I believe these changes to bracket notation can all be reverted once you fix the externs file.

joeyparrish · 2020-03-20T22:44:51Z

ui/controls.js

    video, player, this.config_.castReceiverAppId);

  /** @private {boolean} */
  this.castAllowed_ = true;

  /** @private {HTMLMediaElement} */
-  this.video_ = this.castProxy_.getVideo();
+  this.video_ = this.castProxy_['getVideo']();


Same here. Try reverting these changes once the externs are fixed.

joeyparrish · 2020-03-20T22:44:51Z

ui/controls.js

@@ -675,11 +676,15 @@ shaka.ui.Controls.prototype.addControlsContainer_ = function() {
  this.videoContainer_.appendChild(this.controlsContainer_);

  this.eventManager_.listen(this.controlsContainer_, 'touchstart', (e) => {
-    this.onContainerTouch_(e);
+    if (!e.defaultPrevented) {


This looks like it's probably a useful change independent of the Cast changes. What's the use case? Would you be willing to split that into a separate PR?


        ui seekbar: remember paused state when seeking


        ui seekbar: seek to end needs offset, so events are still triggered


        ui seekbar: fix empty seek bar / glitching when seeking


        Merge branch 'v2.5.x' into feat/ui-custom-castproxy

joeyparrish · 2020-04-20T17:39:42Z

I've just noticed that you're developing against the v2.5.x branch. We normally only accept PRs against master. Cherry-picks to a release branch are always done by the team after carefully considering the risk of a change and any backward-compatibility issues it might create. This does not seem like something we would cherry-pick to an older branch.

If you'd like to continue this PR, please rebase against master and address the feedback you've received so far. Thanks!

tecteun · 2020-04-20T19:06:08Z

Thanks for the explanation, I'll probably handle this rebase and changes the coming week! I couldn't find any time to finish this MR due to the unexpected chain of events. My productivity plummeted, because, you know, kids ;)

joeyparrish · 2020-04-20T19:41:05Z

No worries! We understand, and there's no urgency from our perspective.

googlebot · 2020-04-22T12:21:34Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.


        add shaka.extern.Player definition to avoid mangling proxied function…

… calls, in case of chromecast


        add more missing shaka.extern.Player methods


        fix debug build

Teun van Roovert added 2 commits Mar 13, 2020

Enable custom castproxy in shaka.ui.Overlay

Loading status checks…

b43b098

Allow preventing default for container click / touch

Loading status checks…

478d3d2

joeyparrish requested changes Mar 20, 2020

View changes

Teun van Roovert added 2 commits Apr 20, 2020

ui seekbar: remember paused state when seeking

Loading status checks…

be91e05

ui seekbar: seek to end needs offset, so events are still triggered

Loading status checks…

c356903

tecteun force-pushed the tecteun:feat/ui-custom-castproxy branch from 4629f54 to c356903 Apr 20, 2020

ui seekbar: fix empty seek bar / glitching when seeking

Loading status checks…

9f7f537

tecteun force-pushed the tecteun:feat/ui-custom-castproxy branch from 9ae6a64 to 9f7f537 Apr 20, 2020

Merge branch 'v2.5.x' into feat/ui-custom-castproxy

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

af66d00

tecteun force-pushed the tecteun:feat/ui-custom-castproxy branch 3 times, most recently from b43a60c to af66d00 Apr 22, 2020

tecteun force-pushed the tecteun:feat/ui-custom-castproxy branch from 0d369eb to af66d00 Jul 21, 2020

tecteun added 3 commits Jul 21, 2020

add shaka.extern.Player definition to avoid mangling proxied function…

Loading status checks…

a1de44d

… calls, in case of chromecast

add more missing shaka.extern.Player methods

Loading status checks…

9a0f69c

fix debug build

Loading status checks…

0b513f1

Aug	SEP	Oct
	15
2019	2020	2021

google / shaka-player

Feat/ui custom castproxy #2464

Feat/ui custom castproxy #2464

tecteun commented Mar 20, 2020

googlebot commented Mar 20, 2020

joeyparrish commented Mar 20, 2020

tecteun commented Mar 20, 2020

joeyparrish commented Mar 20, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

joeyparrish commented Apr 20, 2020

tecteun commented Apr 20, 2020

joeyparrish commented Apr 20, 2020

googlebot commented Apr 22, 2020

google / shaka-player

Join GitHub today

Feat/ui custom castproxy #2464

Feat/ui custom castproxy #2464

Conversation

tecteun commented Mar 20, 2020

googlebot commented Mar 20, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

joeyparrish commented Mar 20, 2020

tecteun commented Mar 20, 2020

joeyparrish commented Mar 20, 2020

This comment has been minimized.

joeyparrish Mar 20, 2020

This comment has been minimized.

joeyparrish Mar 20, 2020

This comment has been minimized.

joeyparrish Mar 20, 2020

This comment has been minimized.

joeyparrish Mar 20, 2020

This comment has been minimized.

joeyparrish Mar 20, 2020

joeyparrish commented Apr 20, 2020

tecteun commented Apr 20, 2020

joeyparrish commented Apr 20, 2020

googlebot commented Apr 22, 2020