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

Add PiP(Picture in Picture) mode to the player #98

Merged
merged 12 commits into from Oct 20, 2022

Conversation

suhailkakar
Copy link
Member

Description

Added picture in picture mode to the Player (demo below)

#51

Additional Information

Demo

Screen.Recording.2022-10-14.at.7.28.43.PM.mov

@suhailkakar suhailkakar requested a review from a team as a code owner October 17, 2022 06:31
@vercel
Copy link

vercel bot commented Oct 17, 2022

@suhailkakar is attempting to deploy a commit to the Livepeer Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
livepeerjs ✅ Ready (Inspect) Visit Preview Oct 20, 2022 at 8:31PM (UTC)
livepeerjs-with-next ✅ Ready (Inspect) Visit Preview Oct 20, 2022 at 8:31PM (UTC)
livepeerjs-with-svelte ✅ Ready (Inspect) Visit Preview Oct 20, 2022 at 8:31PM (UTC)

Copy link
Member

@adamsoffer adamsoffer 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 great! Let's resolve those two eslint failures then good to merge after Chase's review.

@0xcadams
Copy link
Member

0xcadams commented Oct 18, 2022

This is a great start! Some things I'd like to see on this:

  • the general architecture (needs documentation) of the video controls is that the store is updated directly and maintains state, and then we add side effects to that store which interacts with the media element based on the diff between states. There should be a good example with Fullscreen which shows this - the button triggers the modification of the state, and then there is a listener which calls functions on the element. Let's change this to handle PiP similarly. I will document this better in the future when I'm back.
  • We will need to test on different browsers and I would generally follow how mux implemented their PiP here (but not directly, since they handle some things we might not care about) https://github.com/muxinc/media-chrome/blob/main/src/js/media-controller.js#L168 I think this PiP request would cause a crash with older browsers as-is, so we'll need to update this
  • Let's add a changeset to this PR which describes the updates made (see historical PRs for examples) to create a new changeset file, run pnpm changeset
  • let's export this component from the main index.ts, so bubble up that export until it's exported from the main index.ts. so someone could use this component in their own custom controls if they wanted to
  • This is more of a question, but should this be included by default with the player? Or should it be, by default, not shown, but optional with some kind of flag (showPipButton)? I'm leaning towards the latter but would love feedback on it!

Thanks for this PR!

Copy link
Member

@0xcadams 0xcadams left a comment

Choose a reason for hiding this comment

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

See above for details

@0xcadams
Copy link
Member

0xcadams commented Oct 18, 2022

Also, to help fix the types, we'll need to update this file with PiP functions. There's some documentation in that global types file which explains what those custom types are, but basically it's typing how historical browsers have defined a "Fullscreen request" in the past. We may not need to add the multiple definitions of "Fullscreen request", depending on what the history is of PiP (I don't know how safari or Firefox have evolved over the years to handle this)
https://github.com/livepeer/livepeer.js/blob/main/global.d.ts

@suhailkakar
Copy link
Member Author

  • Updated the architecture of the picture in picture mode (now it is similar to full screen - store is updated directly and maintains state)

  • Exported the component and added changset

  • The requestPictureInPicture API is not supported by Firefox, therefore to avoid crashes, I have added a condition to not render the picture in picture button if the browser doesn't support it.

image

image

This is more of a question, but should this be included by default with the player? Or should it be, by default, not shown, but optional with some kind of flag (showPipButton)? I'm leaning towards the latter but would love feedback on it!

I think it is better to show the button by default but if the users want they can add disabledPiP={false} to remove it.

Copy link
Member Author

@suhailkakar suhailkakar left a comment

Choose a reason for hiding this comment

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

Fixed the lint issue

Copy link
Member

@0xcadams 0xcadams 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 awesome!! A few more requests please. And I'm not sure I see the reasoning for including PiP by default - it seems like a niche feature that some apps will want, but most won't. I personally don't see it on most video apps I use. It seems to me that it should default to not shown with a documented flag to enable it. Let me know your thoughts!

.changeset/poor-trains-divide.md Outdated Show resolved Hide resolved
global.d.ts Show resolved Hide resolved
packages/core/src/media/controls/controls.ts Outdated Show resolved Hide resolved
packages/core/src/media/controls/pictureinpicture.ts Outdated Show resolved Hide resolved
packages/core/src/media/controls/controls.ts Show resolved Hide resolved
packages/core/src/media/controls/pictureinpicture.ts Outdated Show resolved Hide resolved
@0xcadams
Copy link
Member

Also, can we document this somewhere in the Player docs?

@suhailkakar
Copy link
Member Author

Let's make sure this value is being updated properly on enter/exit of PiP. Users can consume this value to conditionally render UI when PiP is active, so we need to make sure it toggles correctly. I can't test locally currently so just leaving this here to make sure it's tested

@0xcadams I just checked but non of the variables such as full screen, and volume are updating their values from stores.

@0xcadams
Copy link
Member

Let's make sure this value is being updated properly on enter/exit of PiP. Users can consume this value to conditionally render UI when PiP is active, so we need to make sure it toggles correctly. I can't test locally currently so just leaving this here to make sure it's tested

@0xcadams I just checked but non of the variables such as full screen, and volume are updating their values from stores.

True!! Nice catch, thanks for that. Just pushed a fix for that and added old webkit and documentation. Once the CI passes we shall 🚢 🚢 🚢

@0xcadams 0xcadams enabled auto-merge (squash) October 20, 2022 20:38
@0xcadams
Copy link
Member

Merging and seeing if tests pass in main.

@0xcadams 0xcadams merged commit 5fc44a5 into livepeer:main Oct 20, 2022
@github-actions github-actions bot mentioned this pull request Oct 20, 2022
@suhailkakar suhailkakar self-assigned this Nov 1, 2022
@suhailkakar suhailkakar deleted the suhailkakar/PiP-mode branch November 7, 2022 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants