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(UI): Restore missing AirPlay button #7389

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

tykus160
Copy link
Member

@tykus160 tykus160 commented Oct 1, 2024

No description provided.

@tykus160 tykus160 added type: bug Something isn't working correctly component: UI The issue involves the Shaka Player UI priority: P2 Smaller impact or easy workaround browser: Safari Issues affecting Safari or WebKit derivatives labels Oct 1, 2024
@shaka-bot
Copy link
Collaborator

shaka-bot commented Oct 1, 2024

Incremental code coverage: 83.33%

@joeyparrish
Copy link
Member

@avelad texted me and says this "should work fine with remote": "I implemented remote in the past and it works with AirPlay".

@tykus160
Copy link
Member Author

tykus160 commented Oct 1, 2024

@joeyparrish the problem is, remote is always in HTMLMediaElement, so this if/else condition causes no AirPlay button in the UI.

@joeyparrish
Copy link
Member

I think Alvaro is saying that remote should suffice for AirPlay in the UI. I can't confirm this either way.

@joeyparrish
Copy link
Member

You can move forward if you're sure this is right. We trust you. If Alvaro disagrees with this, he can work it out with you when he returns from leave.

@tykus160 tykus160 requested a review from joeyparrish October 2, 2024 07:59
@tykus160
Copy link
Member Author

tykus160 commented Oct 2, 2024

@joeyparrish OK, apparently the issue was with handling of disableRemotePlayback option. Adjusted now, we can use remote button and AirPlay works whenever src= mode is enabled.

@absidue
Copy link

absidue commented Oct 2, 2024

@tykus160 I don't see shaka-player setting it to true anywhere, so is this to work around a Safari bug or is your own code setting disableRemotePlayback to true?

@@ -211,6 +211,7 @@ shaka.media.MediaSourceEngine = class {

return mediaSource;
} else {
this.video_.disableRemotePlayback = false;
Copy link
Member

Choose a reason for hiding this comment

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

My only concern is that we're changing the DOM now. In general, if an app sets disableRemotePlayback to true in the DOM, we shouldn't change that here.

Is the default for disableRemotePlayback true or false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Default is false. We're explictly setting it to true when ManagedMediaSource is in use, I saw otherwise there are some problems even with rendering of demo page.

Copy link
Member Author

Choose a reason for hiding this comment

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

We might stop setting it explicitly at all, but I think it may raise issues when ManagedMediaSource is used.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds good, then.

@tykus160
Copy link
Member Author

tykus160 commented Oct 3, 2024

@absidue @joeyparrish disableRemotePlayback is set to true when ManagedMediaSource is used.
To make our code less invasive, I'm setting it back to false only in src= scenario, MediaSourceEngine has been created and ManagedMediaSource exists.

@tykus160 tykus160 merged commit 96da45a into shaka-project:main Oct 3, 2024
16 of 17 checks passed
@gkatsev
Copy link
Contributor

gkatsev commented Oct 3, 2024

MMS requires disableRemotePlayback to be false unless a second source is added to the video element with the full hls url for airplay. There was this change related to it, but it wasn't ready to be merged: #7136. So, all of this probably would need to be updated once shaka can add that secondary source.

@tykus160 tykus160 deleted the wt-missing-airplay branch October 3, 2024 13:22
@tykus160
Copy link
Member Author

tykus160 commented Oct 3, 2024

@gkatsev I'm aware, but shaka creates MSE before an actual playback and lack of disableRemotePlayback=true causes issues. I'm testing adding secondary source for AirPlay.

@avelad avelad added this to the v4.12 milestone Oct 18, 2024
avelad pushed a commit that referenced this pull request Oct 18, 2024
avelad pushed a commit that referenced this pull request Oct 18, 2024
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Dec 2, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Dec 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
browser: Safari Issues affecting Safari or WebKit derivatives component: UI The issue involves the Shaka Player UI priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants