-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player] Increase seeking performance #5145
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
For the tests, not sure how to test this :( |
Could you elaborate on how this is desirable? If someone seeks to position A, then immediately to position B, why would waiting until buffering is complete at position A to update the seek position when anything buffered at A isn't going to be used? I recommend filing an issue about the underlying issue, since more discussion is needed and it's not obvious that this is the right solution. |
|
I'm confused about the relationship between the issue description and the PR. In the issue you describe the problem being about when the video renders, but here you're waiting on the buffer status. Those aren't the same thing. If the goal is to wait for a single frame to render before seeking again, why are you waiting for buffering to finish? Unless I'm missing something this will make seeking incredibly sluggish on slow network connections, and do a lot of pointless buffering (per my earlier comment). |
Yes, you're right, I confused rendering and buffering in my explanation of the issue. However, the player doesn't wait for the buffer to be full to generate the I'm listening for this event, because, as far as I know, it's the only way to know that something has been paint on the screen. Maybe, we could add a timeout function to unlock the Concerning the pointless buffering, depends on what the user is doing
|
Couldn't you interpose the
It won't fix it, it will just cap it by the timeout. If it takes, on average, X ms to be able to start playing from a new position (or time out), then without your PR it will start playing from B in X ms, but with your PR it will take 2X ms instead.
It is still going to be the case that it will increase the amount of time it takes to play back from the position where the seek bar is finally released by 50% on average.
Couldn't you get the same effect without the delay at the end by rate limiting seek frequency on the Dart side instead? I.e., instead of sending continual seek events while dragging, send them only:
(It wouldn't actually wait for paint events, but if you add a timeout that wouldn't always be the case for the native solution either.) |
Well, you're totally right, it's a more elegant solution. Just implemented it in fluttercommunity/chewie#623 it's working great ! |
This PR prevents calling the
seekTo
functions when the player is already buffering due to a previousseekTo
call, this should enhance the seeking experience by not blocking the preview.Fix: flutter/flutter#101409
What does it do ?
To prevent the issue described in the related PR, we want to limit the number of buffer not leading to an image being displayed.
In order to do this, we just ignore
seekTo
calls when the player is not ready to play.What if we ask for the final location when the preview is buffering ?
This is the reason we store the asked position when we ignore the
seekTo
call, thus,when the play is ready, i.e. it has displayed the new frame, we check if the user have asked for a final location
and render it if needed.
This allow to keep the player responsive even if multiple locations have been asked really quickly.
Try proposed fix
Result
List which issues are fixed by this PR. You must list at least one issue.
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.