[video_player] Improve seek performance on Android#11810
[video_player] Improve seek performance on Android#11810sailendrabathi wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds the backBufferDurationMs option to VideoPlayerOptions to configure the ExoPlayer back buffer duration on Android, updating the platform interface, Android implementation, and adding no-op stubs for AVFoundation and Web. Feedback on the changes highlights that passing backBufferDurationMs via a global setter on Android can cause state persistence and race conditions during concurrent player initialization, and recommends passing this option directly through CreationOptions instead.
b94d8e4 to
624afd5
Compare
624afd5 to
b75044f
Compare
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring ExoPlayer's back buffer duration on Android by introducing backBufferDurationMs to VideoPlayerOptions and forwarding it through the platform interface to the Android implementation. The Android plugin updates PlatformViewVideoPlayer and TextureVideoPlayer to configure ExoPlayer's DefaultLoadControl using this value, supported by new unit tests. Feedback suggests correcting an inaccurate comment regarding how backBufferDurationMs is passed and adding an assertion to ensure the duration is positive if provided.
54e8d1e to
2f8c779
Compare
2f8c779 to
178a8e1
Compare
178a8e1 to
959dbb3
Compare
mboetger
left a comment
There was a problem hiding this comment.
There's multiple formatting issues with this PR.
| /** | ||
| * The duration of the back buffer in milliseconds, used to configure ExoPlayer's load control. | ||
| */ | ||
| public Long backBufferDurationMs; |
There was a problem hiding this comment.
Why a long? Seems like it's an int everywhere else.
There was a problem hiding this comment.
This is to handle the conversion from dart to java.
We keep as a Long in VideoPlayerOptions.java to maintain alignment with Pigeon's generated CreationOptions object.
We then safely clamp the Long into a int at PlatformViewVideoPlayer.java and TextureVideoPlayer.java to match DefaultLoadControl's expected int parameter.
|
You have a bunch of test failures - both formatting and issues stemming from the versioning. You need to follow the instructions here: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins if you want the tests to pass and get a proper review |
2cda4a3 to
673cb55
Compare
|
Only 3 CI tests were failing in the earlier run
|
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring ExoPlayer's back buffer duration on Android by introducing backBufferDurationMs to VideoPlayerOptions. The option is passed from the video_player package through the platform interface to the Android implementation, where it configures the DefaultLoadControl of the ExoPlayer instance. The changes include updated Pigeon-generated files, tests, and example configurations. Feedback on the pull request identifies redundant manual mock initializations in PlatformViewVideoPlayerTest.java that are already handled by the Mockito rule.
673cb55 to
8b87453
Compare
|
@mboetger, only one test failing, with expected failures.
|
mboetger
left a comment
There was a problem hiding this comment.
LGTM - please proceed with the next PR per the federated PR guidlines: https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins
|
@mboetger Please make sure a PR has all the necessary approvals before requesting that it be split up, as the goal is for the sub-PRs to be trivially approved due to already having all the necessary reviews. This still needs an ecosystem-team-level review for the general API changes. |
tarrinneal
left a comment
There was a problem hiding this comment.
Looks solid to me, feel free to re-break out the sub pr's
Adds `backBufferDurationMs` to `VideoPlayerOptions` and configures ExoPlayer's `DefaultLoadControl` to retain the back buffer from keyframes. This significantly improves seek performance and responsiveness during video playback on Android.
649e266 to
3ec63ed
Compare
Adds
backBufferDurationMstoVideoPlayerOptionsand configures ExoPlayer'sDefaultLoadControlto retain the back buffer from keyframes. This significantly improves seek performance and responsiveness during video playback on Android.iOS already has some kind of back buffering implemented in AVPlayer because the replays and seeks are instantaneous for small videos.
Before:
Replays and seeks on Android for network based video assets involve a network call to fetch the video content. This significantly hampers the user experience.
After:
Replays and seeks on Android (within the back buffer duration) are instant just like in iOS. Improving user experience
This PR partly addresses flutter/flutter#97428. Not enabling back buffering by default and instead allowing the app to configure as per need. The default behavior will be same as the existing implementation to avoid any regressions.
Pre-Review Checklist
[shared_preferences]///).