[video_player] Caching for network data source #2429

sanekyy · 2019-12-17T21:19:04Z

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 and maxCacheFileSize to platform only on plugin initialization because it is unnecessary to pass it on each creation of VideoController.
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 called result.success after success which leads to hangs.

Related Issues

flutter/flutter#28094

Checklist

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

# Conflicts: # AUTHORS

# 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

# Conflicts: # AUTHORS

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

…/android/app/libs

aytunch · 2020-11-04T22:53:18Z

Just my opinion but an issue like that is unlikely to get traction. I'd recommend starting a new PR based on this one, ideally after it merges.

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

dnfield · 2020-11-05T16:48:25Z

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.

otopba · 2020-11-05T17:02:26Z

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

dnfield · 2020-11-05T17:51:23Z

Ahh, sorry about that. There's a lot going on these days. I'll try to start reviewing this, but it should probably get a lookover by @cyanglaz and @ditman as well.

dnfield · 2020-11-05T17:52:37Z

...eo_player/video_player/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java

+      VideoPlayerOptions options,
+      long maxCacheSize,
+      long maxCacheFileSize,
+      boolean useCache) {


nit: why have an extra parameter here instead of just setting this based on whether maxCacheSize and maxCacheFileSize > 0?

dnfield · 2020-11-05T17:54:30Z

packages/video_player/video_player/example/lib/main.dart

@@ -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,


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.

dnfield · 2020-11-05T17:54:56Z

packages/video_player/video_player/ios/Classes/FLTVideoPlayerPlugin.m

+
+- (instancetype)initWithURL:(NSURL*)url
+               frameUpdater:(FLTFrameUpdater*)frameUpdater
+                enableCache:(BOOL)enableCache {


dnfield · 2020-11-05T17:55:26Z

packages/video_player/video_player/ios/Classes/FLTVideoPlayerPlugin.m

@@ -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;


dnfield · 2020-11-05T17:56:35Z

packages/video_player/video_player/ios/Classes/messages.m

@@ -20,6 +20,10 @@
                                                    errorDict, @"error", nil];
 }

+@interface FLTInitializeMessage ()


Can we give this a more specific name less likely to cause conflicts with other plugins? E.g. FLTInitializeVideoPlayerMessage?

dnfield · 2020-11-05T17:59:38Z

packages/video_player/video_player/ios/video_player.podspec

@@ -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'


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.

dnfield · 2020-11-05T18:02:41Z

packages/video_player/video_player/lib/video_player.dart

+  /// 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) {


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?

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?

dnfield · 2020-11-05T18:07:36Z

packages/video_player/video_player/lib/video_player.dart

  /// Attempts to open the given [dataSource] and load metadata about the video.
  Future<void> initialize() async {
+    await _ensureVideoPluginInitialized();


I'm hoping @cyanglaz or @ditman can say whether this is breaking. Before, we'd allow multiple calls to this method actually do work - if that was a bug this fixes that's fine, but if not this is breaking.

dnfield · 2020-11-05T18:08:15Z

packages/video_player/video_player/lib/video_player.dart

-  // This will clear all open videos on the platform when a full restart is
-  // performed.
-  ..init();


Where are we doing this now?

Does this code work with a full restart?

dnfield · 2020-11-05T18:09:11Z

packages/video_player/video_player/pigeons/messages.dart

@@ -1,5 +1,10 @@
 import 'package:pigeon/pigeon_lib.dart';

+class InitializeMessage {


@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.

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';

Ahh nice, maybe we want it to be something like FLTVideoPlayer.

gabdsg · 2020-11-25T17:43:45Z

any update on this?

sanekyy · 2020-11-25T17:49:11Z

Hello @gabdsg

Don't have enough time for now.

Will continue working on with PR ASAP.

Zyj0707 · 2020-12-01T06:29:19Z

Hi @sanekyy ,
Hope you have a nice day.
Because the project I am developing now needs to use network video caching, thank you for your help. I forked your branch into my project and found that there are some areas for improvement.

We can increase the parameter of “cacheKey”. Now many caches have developed such parameter settings, because only using url as the cache key will encounter some thorny problems. For example, the video address obtained from the server is changed, and the request is multiple times. The url of the same video address is different. But the cacheKey we keep is the same.
Fortunately, Android exoplayer has such a method to set the cached Key, as follows:

        return new ExtractorMediaSource.Factory(mediaDataSourceFactory)
            .setExtractorsFactory(new DefaultExtractorsFactory())
            .setCustomCacheKey(cacheKey)
            .createMediaSource(uri);

But the VIMediaCache you use in IOS lacks such a way to customize the cache key. I tried to change the source code of
VIMediaCache to achieve this function.
I believe “cacheKey” is still needed for most people.

IOS cache directory is inconsistent with Android. The cache address in Android is video in the cache directory, while IOS is placed in the tmp directory by default. The IOS address can be changed, I hope the two can be consistent.

The above is my suggestion, thank you.

wanjm · 2020-12-07T08:25:12Z

waiting waiting

wanjm · 2021-01-04T11:13:03Z

hurry up, my friends, having waiting for it for long time;
I think it avoid bug in flutter/flutter#61004

cyanglaz · 2021-01-14T19:55:12Z

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.

aytunch · 2021-01-14T20:48:25Z

@sanekyy I know you are busy lately, but could you please sign the CLA? This PR is very important for the community. We can save terabytes of wasted bandwidth :D

Also while you are at it, you could sign the CLA for: #2406

lukepighetti · 2021-01-14T21:45:07Z

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.

#2130
#2406
#2429

See also: #2429 (comment)
and #2130 (comment)

lukepighetti · 2021-01-14T21:47:24Z

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.

lukepighetti · 2021-01-15T02:24:53Z

@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.

ditman · 2021-01-15T02:28:19Z

Here's the info about missing CLAs for this PR (I'm removing email addresses):

user	status
@lukepighetti	need_author_consent
@wwwdata	need_author_consent
@IncludeCmath	need_author_consent
@HiIamAlanOu	need_author_consent
@999eagle	need_author_consent
@yuwen-yan	need_author_consent

lukepighetti · 2021-01-15T02:28:59Z

@cyanglaz I don't have Contributor perms so I can't handle these dangling PRs.

#2130 can be closed
#2406 can be closed
#2429 status unclear until we have a plan for CLA methinks

lukepighetti · 2021-01-15T02:31:32Z

@googlebot I consent.

cyanglaz · 2021-01-15T02:32:21Z

@lukepighetti @sanekyy
I apologize for causing confusions. I closed this PR because it can't be merged due to CLA issues. I'd recommend fixing the CLA issue or fork a new PR. If the CLA issue is fixed, I'm more than happy to re-open it.

ditman · 2021-01-15T02:37:44Z

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 git dependencies, for example).

This PR will need to be split in (at least) 3 different PRs and merged in the correct order (normally): video_player_platform_interface, video_player_web (if it's independent to the video_player code, or backwards compatible), and then the video_player package.

lukepighetti · 2021-01-15T02:38:58Z

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.

lukepighetti · 2021-01-15T02:50:31Z

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 video_player architecture. Perhaps someone can gain insight from it and take another stab at it, but it really should be done from a fresh fork and reimplementation.

ldelafuente · 2021-01-15T02:55:14Z

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.

aytunch · 2021-01-15T08:46:56Z

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.

lukepighetti · 2021-01-15T13:10:19Z

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.

dnfield · 2021-01-15T16:51:05Z

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.

If you are happy with the state of the code in this PR for your own uses, you can apply the patch to a fork and pull it into your project, either via git or by publishing it yourself to pub. Given a number of unresolved issues with this patch, I would not pursue that path myself.
Open another patch in this repo. You will have to make sure you follow our contributing guidelines, which include things like signing a CLA, writing tests for any new functionality, following the Style guide, and potentially engaging in a design document process for larger/more complex features.
Fork this plugin and create your own patch to do this. This will involve maintaining your own fork. If you're doing a good enough job of that, it's even possible to just take over ownership of that plugin as far as the Flutter team is concerned.

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.

2shrestha22 · 2021-09-28T05:29:33Z

Disappointed ...

PhantomRay · 2021-09-28T05:32:04Z

Disappointed ...

I waited for too long. Ended up using better_player, much better.

RezaKhajvand · 2022-12-25T20:15:47Z

there is hope or not?

sanekyy added 30 commits June 10, 2019 23:09

Adds support for holes in polygon overlays to the Google Maps plugin

76d56ca

Add name and contact info to the AUTHORS file

8826721

fix holes generating in example

9252dc4

Merge branch 'master' into polygon_holes

a4eeab5

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

merge with upstream/master

1b26908

Merge remote-tracking branch 'upstream/master' into polygon_holes

01bd55c

# Conflicts: # AUTHORS

Merge remote-tracking branch 'upstream/master' into polygon_holes

69d3889

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md

change version to 0.5.19

18137c2

Merge remote-tracking branch 'upstream/master' into polygon_holes

a86e25f

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md

change version to 0.5.20

e866736

Merge remote-tracking branch 'upstream/master' into polygon_holes

0d404b4

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml

fix format

32ee443

Merge remote-tracking branch 'upstream/master'

71c45b5

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml

Merge branch 'polygon_holes'

4f24a58

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml

fix format

5233c0d

Merge remote-tracking branch 'upstream/master'

a952a34

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md # packages/google_maps_flutter/pubspec.yaml

Merge branch 'master' into polygon_holes

1faf0d4

Merge remote-tracking branch 'upstream/master'

b4ae7bf

# Conflicts: # packages/google_maps_flutter/CHANGELOG.md

Merge branch 'master' into polygon_holes

49d08b3

Merge remote-tracking branch 'upstream/master'

b526132

# Conflicts: # AUTHORS

Merge branch 'master' into polygon_holes

2d55631

Merge remote-tracking branch 'upstream/master'

7871f33

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

Merge branch 'master' into polygon_holes

185f67b

Merge remote-tracking branch 'upstream/master'

14136d5

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

Merge branch 'master' into polygon_holes

4283ce0

# Conflicts: # AUTHORS # packages/google_maps_flutter/CHANGELOG.md

add additional documentation about what is a hole

af1c9b0

migrate to Maps SDK v3

242aa87

add missed maps sdk aar

9255eac

remove unnecessary maps-sdk .arr from android/libs and add to example…

670f24a

…/android/app/libs

add maps sdk v3 back

93535a0

dnfield reviewed Nov 5, 2020

View reviewed changes

cyanglaz closed this Jan 14, 2021

		@@ -1,5 +1,10 @@
		import 'package:pigeon/pigeon_lib.dart';

		class InitializeMessage {

[video_player] Caching for network data source #2429

[video_player] Caching for network data source #2429

Conversation

sanekyy commented Dec 17, 2019 • edited

Description

Related Issues

Checklist

Breaking Change

aytunch commented Nov 4, 2020

dnfield commented Nov 5, 2020 • edited

otopba commented Nov 5, 2020

dnfield commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabdsg commented Nov 25, 2020

sanekyy commented Nov 25, 2020

Zyj0707 commented Dec 1, 2020

wanjm commented Dec 7, 2020

wanjm commented Jan 4, 2021 • edited

cyanglaz commented Jan 14, 2021

aytunch commented Jan 14, 2021 • edited

lukepighetti commented Jan 14, 2021 • edited

lukepighetti commented Jan 14, 2021

lukepighetti commented Jan 15, 2021 • edited

ditman commented Jan 15, 2021

lukepighetti commented Jan 15, 2021 • edited

lukepighetti commented Jan 15, 2021

cyanglaz commented Jan 15, 2021

ditman commented Jan 15, 2021 • edited

lukepighetti commented Jan 15, 2021 • edited

lukepighetti commented Jan 15, 2021 • edited

ldelafuente commented Jan 15, 2021 • edited

aytunch commented Jan 15, 2021 • edited

lukepighetti commented Jan 15, 2021 • edited

dnfield commented Jan 15, 2021

2shrestha22 commented Sep 28, 2021

PhantomRay commented Sep 28, 2021

RezaKhajvand commented Dec 25, 2022

sanekyy commented Dec 17, 2019 •

edited

dnfield commented Nov 5, 2020 •

edited

wanjm commented Jan 4, 2021 •

edited

aytunch commented Jan 14, 2021 •

edited

lukepighetti commented Jan 14, 2021 •

edited

lukepighetti commented Jan 15, 2021 •

edited

lukepighetti commented Jan 15, 2021 •

edited

ditman commented Jan 15, 2021 •

edited

lukepighetti commented Jan 15, 2021 •

edited

lukepighetti commented Jan 15, 2021 •

edited

ldelafuente commented Jan 15, 2021 •

edited

aytunch commented Jan 15, 2021 •

edited

lukepighetti commented Jan 15, 2021 •

edited