[video_player_platform_interface] Improve seek performance on Android#11932
[video_player_platform_interface] Improve seek performance on Android#11932sailendrabathi wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces backBufferDurationMs to VideoPlayerOptions to configure ExoPlayer's back buffer duration on Android, and adds videoPlayerOptions to VideoCreationOptions. Feedback suggests updating the assertion to allow a value of 0 (enabling developers to disable back buffering) and correcting the markdown spacing in the documentation comment for proper bold rendering.
c686c5b to
3360f98
Compare
mboetger
left a comment
There was a problem hiding this comment.
Holding on for ecosystem review
3360f98 to
7811816
Compare
|
@mboetger, I updated the PR after approval from ecosystem reviewer. Comment link for reference: #11810 (review) |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| /// Additional web controls | ||
| final VideoPlayerWebOptions? webOptions; | ||
|
|
||
| /// ** Android only **. Sets ExoPlayer's back buffer duration in milliseconds. |
There was a problem hiding this comment.
We don't make assertions about what platforms implement something in platform interface packgaes, because this package doesn't control that. What if a third-party implementation for another platform supports it, for instance? Or it's added to iOS later, without any change to this package?
We also don't describe behavior in terms of underlying SDK details unless there's no other way to. This comment should describe what the back buffer is, conceptually, without reference to ExoPlayer.
| /// ** Android only **. Sets ExoPlayer's back buffer duration in milliseconds. | ||
| /// | ||
| /// This is not possible on iOS because AVPlayer does not provide a way to | ||
| /// configure the back buffer duration. |
There was a problem hiding this comment.
Same here. You can say "Ignored on platforms that do not support controlling the back buffer.", which is both generic to other implementations, and future-proof.
There was a problem hiding this comment.
I was going to say this, but I figured once the interface is reworked to use the new standard of supportsX I would remove it myself.
There was a problem hiding this comment.
but Stuart is right, that is protocol
There was a problem hiding this comment.
It's not just protocol; this class is directly re-exported from the app-facing package currently, so these will be client-facing docs as soon as they land.
| @@ -1,3 +1,7 @@ | |||
| ## 6.9.0 | |||
|
|
|||
| * Adds `backBufferDurationMs` to `VideoPlayerOptions` to support configuring ExoPlayer back buffer duration on Android. | |||
There was a problem hiding this comment.
Same here; don't reference ExoPlayer or Android.
| final VideoViewType viewType; | ||
|
|
||
| /// Additional configuration options for the video player. | ||
| final VideoPlayerOptions? videoPlayerOptions; |
There was a problem hiding this comment.
Composing the two different options classes seems potentially confusing, but it doesn't look like the API surface ever clearly distinguished these conceptually. Since VideoCreationOptions isn't exposed to app-level clients it's probably not a big deal.
@tarrinneal As part of revisiting the video_player API surface later, it would be good to better distinguish what classes are for what steps of the process (e.g., global vs player-level options, which may have been the original intent here?), and we should also fix the fact that some of these classes are being directly re-exported from the app-facing package, which we should stop doing.
Adds
backBufferDurationMstovideo_player_platform_interfaceThis PR has only the platform interface package changes from the main PR: #11810.
Following PR split as per Changing federated plugins
Pre-Review Checklist
[shared_preferences]///).