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

[Android] Add a delay before displaying progress indicator #623

Merged

Conversation

henri2h
Copy link
Contributor

@henri2h henri2h commented Apr 1, 2022

After a bit of digging in the video_player, I managed to enhance the seeking performance.

However, with this when seeking, it's frequent to have dozen of milliseconds between STATE_BUFFERING and STATE_READY. This makes the CircularProgressIndicator be displayed during a few milliseconds.

To prevent this, we propose to introduce a bit of temporization between the first isBuffering status and displaying the CircularProgressIndicator.

Proposal Actual
chewie schnell

Currently, an arbitrary timeout of 200ms has been chosen.

Linked pull request : flutter/plugins#5145

NB: This proposal only edit the material_control version of the player. I can edit the other view if you are willing to accept this proposal.

@diegotori
Copy link
Collaborator

@henri2h so that linked PR is currently awaiting testing, so until that one gets merged into the next video_player release, I'm gonna hold off on this. We also have to determine if the same change in that linked PR will be made to iOS, since right now, this only affects Android.

@diegotori
Copy link
Collaborator

However, if that Linked PR doesn't get approved, then I'll review. Can you modify the example app so that it's able to play content that evaluates this proposed change?

@diegotori
Copy link
Collaborator

Ideally speaking, if this was controlled by the ChewieController, so that the user can determine whether to enable this functionality, that would also be ideal.

@henri2h
Copy link
Contributor Author

henri2h commented Apr 1, 2022

However, if that Linked PR doesn't get approved, then I'll review. Can you modify the example app so that it's able to play content that evaluates this proposed change?

Will do this on monday :)

@henri2h henri2h force-pushed the temporized-buffering-status branch from dc356b5 to 1f51318 Compare April 4, 2022 09:31
@henri2h
Copy link
Contributor Author

henri2h commented Apr 4, 2022

Lint is failing due to import from git repo but that's due to waiting for the upstream PR

@henri2h
Copy link
Contributor Author

henri2h commented Apr 11, 2022

After some discussion, it was pointed out that the same effect could be achieved in the callback of the seekbar which is simpler and more elegant.

So this PR doesn't require anymore external PR.

@diegotori what do you think ? Do you want me to add the support for the CirculaProgressIndicator display delay to the other theme ?

@henri2h henri2h changed the title Add a delay before displaying progress indicator [Anrdoid] Add a delay before displaying progress indicator Apr 11, 2022
@henri2h henri2h changed the title [Anrdoid] Add a delay before displaying progress indicator [Android] Add a delay before displaying progress indicator Apr 11, 2022
@diegotori
Copy link
Collaborator

@henri2h is this occurring on iOS as well? If so, it would make sense to also add the delay there if and only if it occurs on iOS.

@henri2h
Copy link
Contributor Author

henri2h commented Apr 12, 2022

@henri2h is this occurring on iOS as well? If so, it would make sense to also add the delay there if and only if it occurs on iOS.

I don't think so, but I don't have a mac so can't really test it.

@diegotori
Copy link
Collaborator

In that case, I'll have to take a look at it later today.

example/lib/app/app.dart Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
example/lib/app/app.dart Outdated Show resolved Hide resolved
@@ -302,9 +310,78 @@ class _ChewieDemoState extends State<ChewieDemo> {
),
],
),
if (Platform.isAndroid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not using it for iOS and Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any apple device so I can't test on iOS and don't want to modify things that I'm not sure it properly works / is needed.

example/lib/app/app.dart Outdated Show resolved Hide resolved
@diegotori
Copy link
Collaborator

LGTM

@diegotori
Copy link
Collaborator

@brianegan I'm curious about your thoughts on this change, whenever you're able to chime in. Thanks in advance.

Copy link
Collaborator

@brianegan brianegan left a comment

Choose a reason for hiding this comment

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

Overall I think the solution makes sense, but it would be cool to see it applied to both Cupertino and Material controls. That will ensure feature parity and make future maintenence easier.

example/lib/app/app.dart Outdated Show resolved Hide resolved
lib/src/chewie_player.dart Outdated Show resolved Hide resolved
@diegotori
Copy link
Collaborator

Please re-sync your changes with master's current changes. Thanks.

@diegotori
Copy link
Collaborator

Once you re-sync the branch, I'm gonna attempt to smoke test these changes on my end, in particular with an iOS Simulator. If all works well after that, then I would say this PR is ready for release.

@henri2h henri2h force-pushed the temporized-buffering-status branch from 1e62c9f to 79bfde8 Compare May 19, 2022 13:20
@henri2h
Copy link
Contributor Author

henri2h commented Jun 1, 2022

Changes re synced with master, did you had time to review the changes ? Thanks !

@diegotori
Copy link
Collaborator

@henri2h not yet. Will attempt to do so today or tonight. Thank you for your patience.

@diegotori
Copy link
Collaborator

@henri2h After running your changes, looks like the webm file in the example app is not loading in iOS at all. Apparently, the iOS video_player doesn't seem to like webm files. Can you please replace it with an alternate video, preferably in MP4 format, so that I can further evaluate your changes? Thanks in advance.

@diegotori
Copy link
Collaborator

diegotori commented Jun 3, 2022

However, when I replaced the suspect video link with this one, it did work. So far no issues when I used that video.

Please edit the example app and replace the current webm video with either this one or another one of your choice. Thanks.

@henri2h
Copy link
Contributor Author

henri2h commented Jun 7, 2022

However, when I replaced the suspect video link with this one, it did work. So far no issues when I used that video.

Please edit the example app and replace the current webm video with either this one or another one of your choice. Thanks.

Done ;)

@diegotori
Copy link
Collaborator

LGTM. Thanks for all of your hard work, @henri2h.

@diegotori diegotori merged commit 7b3b839 into fluttercommunity:master Jun 8, 2022
@henri2h
Copy link
Contributor Author

henri2h commented Jun 8, 2022

thanks!

diegotori added a commit that referenced this pull request Jun 21, 2022
codenameakshay pushed a commit to qoohoo-app/chewie that referenced this pull request Aug 29, 2022
lg8294 added a commit to lg8294/chewie that referenced this pull request Sep 22, 2022
* commit '3948d23733638d8a58ea5ff065abfe96934d2373': (110 commits)
  Version 1.3.4. Addresses PRs fluttercommunity#623 and fluttercommunity#646.
  Update pubspec.yaml
  version updates
  fix: change video type
  fix: format and migration artifacts
  fix: apply diegotori suggestion
  feat: added support for apple and desktop version
  feat: Make progressIndicatorDelay a Duration
  fix: simplify assignation
  fix: name
  fix: let the android player buffer before seeking once more
  fix: removed old code
  fix: lint
  feat: added method to set progress delay
  feat: added example debug
  feat: add a delay before displaying progress indicator
  Version 1.3.3. Adds PR fluttercommunity#634.
  Update pubspec.yaml
  chore: remove unused dependency
  Version 1.3.2. Adds PR fluttercommunity#626.
  ...

# Conflicts:
#	example/ios/Runner.xcodeproj/project.pbxproj
#	example/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
#	example/ios/Runner/Info.plist
#	example/lib/main.dart
#	example/pubspec.yaml
#	lib/src/chewie_player.dart
#	pubspec.yaml
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.

4 participants