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

Migrate Video Player #1466

Merged
merged 8 commits into from
Aug 22, 2024
Merged

Migrate Video Player #1466

merged 8 commits into from
Aug 22, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Jun 23, 2024

Pull Request Description

This draft PR migrates the existing river_player with Flutter's video_player. The main reason for this is to use an actively maintained video player package. Since the video_player package does not come with built-in controls, I've created new widgets to handle most, if not all of the currently available features. This includes the following settings:

  • Auto fullscreen (with a caveat)
  • Muting videos on load
  • Looping videos
  • Video autoplay
  • Default playback speed

To mimic auto-full screen when the device is in portrait-locked mode, I rotate the video player by 90 degrees. This means that the video player doesn't account for the landscape orientation of the phone (this is only while the device is portrait-locked)

TODO:

  • Auto-hide controls when video is playing

Issue Being Fixed

Issue Number: #1442

Screenshots / Recordings

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu marked this pull request as ready for review July 1, 2024 20:52
@micahmo
Copy link
Member

micahmo commented Aug 12, 2024

Hey @hjiangsu I was just going to comment on this PR that I had run across another Flutter app which uses video_player directly with their own controls on top of it. I was going to link their code in case it had anything helpful.

https://github.com/j-fbriere/squawker/blob/master/lib/tweet/_video_controls.dart

Then I noticed that you had already marked this PR ready for review? Is that still the case? Would it be worth taking it for a test?

@hjiangsu
Copy link
Member Author

Ahh yeah, this PR is ready for review but I was going to wait until 0.6.0 to merge it in! I think we can take a look at how their implementation looks like, and take ideas to improve on the current implementation I have 😅

@micahmo
Copy link
Member

micahmo commented Aug 12, 2024

Just to double-check, your video player is not being used for YouTube videos, right? We would have to introduce a new step which retrieves the raw video URL to pass to the player?

@hjiangsu
Copy link
Member Author

Just to double-check, your video player is not being used for YouTube videos, right? We would have to introduce a new step which retrieves the raw video URL to pass to the player?

Correct! This is only currently being used for the general video player but I would like to extend this to YouTube videos as well.

One idea we can do here is to use a package like https://pub.dev/packages/youtube_explode_dart to possibly retrieve metadata for a given YouTube url (without the need to provide an API key). However, my concern with using something like youtube_explode_dart is that it can be subject to break if YouTube itself changes their internal logic.

A (potentially) more robust but less trivial solution would be to convert the youtube link to a piped link (or similar) and retrieve video metadata there. If there's no dart library for this, we might have to build one ourselves to call the proper API endpoints.

@micahmo
Copy link
Member

micahmo commented Aug 12, 2024

Sounds good! You should be good to merge this after the next general release and we can start getting our hands on it.

@hjiangsu hjiangsu merged commit 3aa493c into develop Aug 22, 2024
1 check passed
@hjiangsu hjiangsu deleted the feature/migrate-video-player branch August 22, 2024 19:33
@hjiangsu
Copy link
Member Author

Just merged this in - let me know if you encounter any quirks when using this!

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