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

Fixed pop the wrong page when changing the speed and added support for multiple resolutions #867

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

akmalova
Copy link

@akmalova akmalova commented Oct 28, 2024

1. Fixed pop the wrong page when changing the speed
Issue: #618

2. Added support for multiple resolutions
Issue: #852

Now, to be able to change the video resolution, you need to pass the following parameters to the ChewieController:

  1. allowQualityChanging: true
  2. Map
qualities: {
        '240p': '<your link to a 240p video>',
        '360p': '<your link to a 360p video>',
        '720p': <your link to a 720p video>',
        '1080p': '<your link to a 1080p video>',
      }

The key can be any string that you want to display in the list of resolutions in the player. The value should be link to the video of this resolution.

Also, in order to be able to change the video resolution, I need to redefine the videoPlayerController inside the ChiewieController. Therefore, the onVideoControllerChange callback has been added, which will allow you to get a new controller when it is replaced.

The quality change has been added for iOS and Android only.

@diegotori
Copy link
Collaborator

@akmalova Thanks for your contribution.

I do have a major concern with changing the resolution. Since this is driven primarily by VideoPlayerController, shouldn't it be relegated to re-creating the ChewieContoller instead of internally mutating the VideoPlayerController? Just trying to wrap my head regarding this change, because the change that you're proposing represents a fundamental shift in the way ChewieController behaves.

@brianegan and/or @Ahmadre, I would like your thoughts on the above.

Also, for the fix that addresses #618, if you can please split that one up into a separate PR, that would be easier for me to review and eventually approve, assuming any concerns that I may raise are eventually addressed.

Thanks in advance.

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.

2 participants