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 more playback rates #1073

Conversation

santiwanti
Copy link
Contributor

The Youtube Web Client supports any value between 0.25 and 2.0 so I'm guessing this API should be able to do so as well. I have added two commits, one that adds just the missing 0.75, 1.25, and 1.75 increments and one that adds a function to allow any float value. We might have to check what happens if 2.5 or 100 is sent and maybe a doc indicating that only values between 0.25 and 2.0 are valid.

This PR addresses:
#1001
#992

@PierfrancescoSoffritti
Copy link
Owner

Hi, thanks for the PR!

Did you try passing a value bigger than 2? what happens?

@PierfrancescoSoffritti PierfrancescoSoffritti changed the base branch from master to dev October 26, 2023 05:05
@santiwanti
Copy link
Contributor Author

I just tested it which lead me to fix a few bugs I had introduced.

When calling it with a value greater than 2 the playback rate is set to 2. Should we add a require making sure the user never sends a value greater than 2 or less than 0.25? Or should we document the behaviour indicating that any value outside the range 0.25 and 2 will be coerced so 0.1 or negative numbers will be 0.25 and 3 will be 2?

@PierfrancescoSoffritti
Copy link
Owner

When calling it with a value greater than 2 the playback rate is set to 2

Based on this, I'd say we should not add this method fun setPlaybackRate(playbackRate: Float). It seems to do the same thing the other method does.

@santiwanti
Copy link
Contributor Author

santiwanti commented Oct 27, 2023

The difference between fun setPlaybackRate(playbackRate: Float) and fun setPlaybackRate(playbackRate: PlayerConstants.PlaybackRate) is that for values like 0.45 it will return 0.45 instead of PlayerConstants.PlaybackRate.UNKNOWN

@santiwanti santiwanti marked this pull request as ready for review October 31, 2023 10:29
@santiwanti
Copy link
Contributor Author

@PierfrancescoSoffritti sorry to bother you. Anything else that should be done on this PR?

@PierfrancescoSoffritti
Copy link
Owner

I have removed the option to use a generic float to set the playback rate, as it doesn't seem necessary and introduces too much ambiguity in the API.

But happy to add the new enums, thanks for your contribution :)

@PierfrancescoSoffritti PierfrancescoSoffritti merged commit 0cc0b52 into PierfrancescoSoffritti:dev Nov 12, 2023
@santiwanti
Copy link
Contributor Author

@PierfrancescoSoffritti the point of being able to set the playback rate with an arbitrary float is that you can set the value to 1.2 or any other value between 0.25 and 2.0 which doens't have an enum value. This is also allowed in the youtube website if you click on playbackrate and select custom.

@PierfrancescoSoffritti
Copy link
Owner

I understand, why is using the predefined values not enough for your use case?

@santiwanti
Copy link
Contributor Author

In my use case we have to allow any value between 0.25 and 2.0 with increments of 0.01. This is for an app for musicians which might want to play the video at arbitrary speeds. I understand if you don't want to implement this. For now I have copied my fork into the project and use that.

@PierfrancescoSoffritti
Copy link
Owner

I see. I think wanting to add a function to set an arbitrary playback speed is a reasonable request.

I will add the method, but it will take some time as I need to make sure the API we are introducing is clear and won't lead to confusing behavior. (And I have limited time to spend on the library)

I have opened #1081 to keep track of this, I will get to it before the end of the year.

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