-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[video_player] Caching for network data source #2429
Conversation
# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # AUTHORS
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml
# Conflicts: # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # AUTHORS
# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md
# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md
…/android/app/libs
I hope this gets merged this week. It has taken much longer than necessary. Once it does, I will do as you said and start a new PR |
We cannot review, let alone merge, this PR until its merge conflicts are fixed up. It will also need appropriate tests to be merged. If the original author is not available to fix this, I would recommend someone who is interested and able take it over. |
The author constantly corrects conflicts, but you so rarely enter this PR that new conflicts appear |
VideoPlayerOptions options, | ||
long maxCacheSize, | ||
long maxCacheFileSize, | ||
boolean useCache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why have an extra parameter here instead of just setting this based on whether maxCacheSize
and maxCacheFileSize
> 0?
@@ -221,6 +220,7 @@ class _BumbleBeeRemoteVideoState extends State<_BumbleBeeRemoteVideo> { | |||
'https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4', | |||
closedCaptionFile: _loadCaptions(), | |||
videoPlayerOptions: VideoPlayerOptions(mixWithOthers: true), | |||
useCache: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here - it seems like it's probably fine to opt users into caching as long as the max values are set and we document what they are/how to change them.
|
||
- (instancetype)initWithURL:(NSURL*)url | ||
frameUpdater:(FLTFrameUpdater*)frameUpdater | ||
enableCache:(BOOL)enableCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -521,8 +546,20 @@ - (FLTTextureMessage*)create:(FLTCreateMessage*)input error:(FlutterError**)erro | |||
player = [[FLTVideoPlayer alloc] initWithAsset:assetPath frameUpdater:frameUpdater]; | |||
return [self onPlayerSetup:player frameUpdater:frameUpdater]; | |||
} else if (input.uri) { | |||
player = [[FLTVideoPlayer alloc] initWithURL:[NSURL URLWithString:input.uri] | |||
frameUpdater:frameUpdater]; | |||
BOOL useCache = input.useCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -20,6 +20,10 @@ | |||
errorDict, @"error", nil]; | |||
} | |||
|
|||
@interface FLTInitializeMessage () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give this a more specific name less likely to cause conflicts with other plugins? E.g. FLTInitializeVideoPlayerMessage
?
@@ -17,6 +17,7 @@ Downloaded by pub (not CocoaPods). | |||
s.source_files = 'Classes/**/*' | |||
s.public_header_files = 'Classes/**/*.h' | |||
s.dependency 'Flutter' | |||
s.dependency 'VIMediaCache' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid taking on a dependency for this?
In particular, it's concerning that this dependency hasn't been updated for 3 or 4 years.
/// Cache used only for network data source. | ||
/// | ||
/// Throws StateError if you try to set the cache size twice. You can only set it once. | ||
static void setCacheSize(int maxSize, int maxFileSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is a little confusing to me. Why don't we just make this part of intialize
below, with optional named parameters, or alternatively make it part of the .network
ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also document what happens if this value changes from initialization to initialization.
Is the cache per controller or per application?
If the max size changes, do we trim the cache immediately or when we try to add more to it? Or do we not trim it at all?
How can we evict a file from the cache? How can we evict all files from the cache?
/// Attempts to open the given [dataSource] and load metadata about the video. | ||
Future<void> initialize() async { | ||
await _ensureVideoPluginInitialized(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This will clear all open videos on the platform when a full restart is | ||
// performed. | ||
..init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are we doing this now?
Does this code work with a full restart?
@@ -1,5 +1,10 @@ | |||
import 'package:pigeon/pigeon_lib.dart'; | |||
|
|||
class InitializeMessage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaaclarke might know if there's some way to tell pigeon to prefix or postfix class names to avoid clashes, or if I'm just being paranoid about this.
My concern is about this generating FLTInitializeMessage
in the objc implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the objc generator there is a flag that allows you to add a prefix to generated classes. If you look a couple lines lower we are specifying FLT as the prefix in this line: opts.objcOptions.prefix = 'FLT';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh nice, maybe we want it to be something like FLTVideoPlayer
.
any update on this? |
Hello @gabdsg Don't have enough time for now. Will continue working on with PR ASAP. |
Hi @sanekyy ,
But the VIMediaCache you use in IOS lacks such a way to customize the cache key. I tried to change the source code of
The above is my suggestion, thank you. |
waiting waiting |
hurry up, my friends, having waiting for it for long time; |
I'm going to close the PR because the CLA is not signed. Feel free to ping me when the CLA is signed and I'll re-open the PR. Or you can open a new PR after signing the CLA. |
There are three PRs with varying CLA status. I cannot see the CLA bot status to see who is missing, but here are the three PRs that I'm aware of. If the bot is looking for CLA from the first PR, those are complete, but in a different PR. See also: #2429 (comment) |
I've been babysitting this PR for 15 months. I have never in my life seen such a complete disregard for the time of contributors. I'm done. |
@Hixie @cyanglaz Can we come up with a plan to resolve CLA? I think @999eagle is not available to sign CLA. I can sign another CLA. @sanekyy might be available to sign another CLA? Can someone override the CLA bot after a manual review? If we can resolve that I think we will have a clear path to victory. I don't need this feature anymore but it's clear that there are many in here who do. |
Here's the info about missing CLAs for this PR (I'm removing email addresses):
|
@googlebot I consent. |
@lukepighetti @sanekyy |
Also, it seems this PR touches all video_player_* packages, but those can't be merged all at the same time (we can't publish This PR will need to be split in (at least) 3 different PRs and merged in the correct order (normally): |
Honestly I don't think this thing is going to get across the finish line if that's the case. It's way too out of sync with master from being so old at this point. |
I know this isn't my PR but I've been following it from day one and I feel I can take responsibility for saying it's time to kill this PR. I don't have the bandwidth to get this thing across the finish line and I'm pretty confident that no one else will. Even if CLA issues get resolved it still needs a lot of work to line up with the current |
I agree with @lukepighetti. Even though we welcome all the work done on this PR, it's been way too long. Sadly, I haven't gained much knowledge about its inner workings, so it would be great if someone else takes over this project starting with a fresh PR. |
I have been following and waiting for this very basic feature to be a part of the video player for a long time. I am sad that contributors/authors of this PR are fed up with the lack of interest from the google team. I understand that they now lost interest to go forward with this PR. If this was taken care of a year ago, there would not be such extra work needed to merge this. You can not expect community developers to take action for some code they wrote months ago when you or a bot feels like it. For a video extensive app done with Flutter, here are the current two options: 1- since there is no caching, every single time app is opened, user will download the same videos over and over again. This results in excessive Firebase bills. Only 1 GB bandwidth is free per day. End-users of this app when checking the bandwidth use instantly deletes the app from their phones. 2- integrate flutter video cache which is basically downloading the videos before supplying them to video player. So no setdatasource.network but always setdatasource.file. This drops the bandwidth use substiantilly but if a user would have only streamed the first 2 seconds of a video and passed to the next video, with this strategy we are still downloading the whole file. Dart http does not support partial downloads and interestingly not even canceling an ongoing download. So still wasted bandwidth but not as much. However from end-users point of view, when the apps video feed opens, they will see the circular progress indicator for a long time waiting for the whole video to be downloaded instead of instantly starting to stream the video So what I am saying is, there is a working PR, and if because of the late interest of google there are now extra steps involved to merge it, than google can take it from here and move some resources for this subject. |
Probably the best way forward for the community is to release this as a package and hope it rots slowly enough to work until someone can take a fresh look at the problem and do a new PR. Unfortunately I won't be able to commit to managing said package. |
It's really unfortunate that this PR went so long without getting a review. I know a number of people have either worked on this PR or eagerly awaited it landing. I'm part of that group, having taken some time to review it, although I certainly only became aware of it months after it was originally opened. Having the PR closed without resolution is disappointing. This repository can't accept PRs that have CLA issues. It also can't accept PRs without a positive review - more importantly, a PR that has outstanding issues that will cause more work for Flutter contributors both inside and outside of Google. (This is what we refer to as "the team".) It also creates headaches for users if they end up with a problematic implementation. Finally, we can't leave PRs open that are not actively being worked on. See our process. IMHO, this PR should have been closed much earlier than it was, if nothing else until the CLA issues were resolved. @cyanglaz did the right thing here. I should have done it myself sooner when there was no response from the author(s). One other thing is that this patch is acutally tackling a non-trivial, somewhat challenging area, and trying to do it for multiple platforms. That's great, but it certainly increases the difficulty and time involved in getting it to land. Landing this feature in this repository will require non-trivial amounts of work. It may be more work than someone who has other professional responsibilities is willing to take on. The team's approach to this is that it would be better to wait until someone is available (whether a current or new team member) to drive this to completion than to accept a problematic patch. We have several options to move this forward.
I understand that last option is a lot of work. You may not want to do that work. That's fine. Just bear in mind that it's still a lot of work to land such a patch in this repository. Please feel free to discuss this further in the #hackers-ecosystem chat or the #hackers chat - see chat. |
Disappointed ... |
I waited for too long. Ended up using better_player, much better. |
there is hope or not? |
Description
This PR improves other PR to speed up the process of merging of this feature
Adapt caching to the new structure of video plugin.
Add tests.
Pass
maxCacheSize
andmaxCacheFileSize
to platform only on plugin initialization because it is unnecessary to pass it on each creation ofVideoController
.Pass
useCache
property to platform only for network data source.Fix bug with
disposeAllPlayers
which wasn't called on plugin initialization as intended.Also on platform side in
init
wasn't calledresult.success
after success which leads to hangs.Related Issues
flutter/flutter#28094
Checklist
///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?