[video_player] Added new field allowBackgroundPlayback to enable background playing of video #4324

IlyaMax · 2021-09-08T17:28:58Z

Description
This PR introduces a new option to VideoPlayerOptions, which enables user to decide on whether user wants to keep playing video in the background or not.
This is a much requested feature, in many issues.

Related issues
63836 - flutter/flutter#63836
63735 - flutter/flutter#63735
55673 - flutter/flutter#55673
48008 - flutter/flutter#48008
40722 - flutter/flutter#40722
32571 - flutter/flutter#32571

Pre-launch Checklist

google-cla · 2021-09-08T17:29:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

IlyaMax · 2021-09-08T17:31:18Z

@googlebot I signed it!

stuartmorgan-g · 2021-09-09T20:11:34Z

Thanks for the submission! We’re currently working through a large backlog of PRs, and this will require non-trivial review, so it will take some time before we’re able to review it. As explained in CONTRIBUTING.md, votes for the corresponding issue are the primary way we’re prioritizing non-trivial reviews, so we encourage anyone interested in this PR to vote for the corresponding issue.

ZhangZhiH · 2021-09-28T12:37:54Z

@IlyaMax
hello
All I see is an observeAppLifecycle
But I don't see background playback implemented there
How to play video in background

IlyaMax · 2021-10-04T09:57:24Z

@ZhangZhiH audio playback is active when you close application or lock screen. if you mean picture in picture, it is not related.

ZhangZhiH · 2021-10-07T12:51:41Z

@ZhangZhiH audio playback is active when you close application or lock screen. if you mean picture in picture, it is not related.

Yeah, I want the video to keep playing after I lock the screen
But I can't implement this feature right now
I currently have video_Player version 1.0.1
Is there anything I can do
I have looked at the changes you submitted and found that this feature is also not implemented

ZhangZhiH · 2021-10-10T08:44:07Z

@ZhangZhiH audio playback is active when you close application or lock screen. if you mean picture in picture, it is not related.

This problem now finds a solution
Use audio player when switching lock screen
Change back to the video player when cutting back to the page and synchronize the progress

stuartmorgan-g

Overall this change seems reasonable, it'll just need more meaningful testing.

stuartmorgan-g · 2021-12-16T18:36:50Z

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart

+  /// Set this to true to let plugin manage play/pause with app lifecycle changes,
+  /// Set this to false to keep playing video in background, when app goes in background
+  /// The default value is true
+  final bool observeAppLifecycle;


I think it would make more sense to call this pauseInBackground or similar; observing the app lifecycle is how it's implemented, not the actual user intent of the option.

maybe needBackgroundPlayback?

I'm not sure about "need", but allowBackgroundPlayback sounds good to me.

stuartmorgan-g · 2021-12-16T18:40:11Z

packages/video_player/video_player/test/video_player_test.dart

+        File(''),
+        videoPlayerOptions: VideoPlayerOptions(observeAppLifecycle: true));
+    await controller.initialize();
+    expect(controller.videoPlayerOptions!.observeAppLifecycle, true);


This is testing that the options struct isn't modified when it's passed it, but that's all; if your changes to initialize() that honor this option were reverted the test would still pass. This needs a real test that the option is being honored.

You mean I need to validate _VideoAppLifeCycleObserver doesn't work if flag is specified?

The purpose of a test is to verify that a change works. If I can remove the key part of a change and the test would still pass (which is true here), then the test isn't actually testing the feature.

I haven't tried this, but I believe you could call WidgetsBinding.instance!.handleAppLifecycleStateChanged(AppLifecycleState.paused) in the test and assert that it doesn't pause. You should similarly add a test that does that with the option set the other way and ensure that it does.

blessedman · 2021-12-18T06:07:05Z

It looks like background playback will be supported soon

stuartmorgan-g

Some nits, but this looks good overall.

@gaaclarke for secondary review before we split out the platform interface change.

stuartmorgan-g · 2022-01-21T18:04:27Z

packages/video_player/video_player/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.3.0
+
+* Add `allowBackgroundPlayback` to `VideoPlayerOptions`


Nit: Adds

(Per new repo CHANGELOG style guide)

stuartmorgan-g · 2022-01-21T18:07:22Z

packages/video_player/video_player/test/video_player_test.dart

 void main() {
+  void verifyAppLifeCycle(
+    VideoPlayerController controller, {
+    bool isObserving = true,


Please make this required instead of having a default value; it's better for tests to be as explicit as possible.

stuartmorgan-g · 2022-01-21T18:07:45Z

packages/video_player/video_player/test/video_player_test.dart

+        await controller.play();
+        verifyAppLifeCycle(controller);
+      });
      test('asset', () async {


Nit: blank line between tests please.

stuartmorgan-g · 2022-01-21T18:08:55Z

packages/video_player/video_player/test/video_player_test.dart

+      expect(controller.videoPlayerOptions!.mixWithOthers, true);
+    });
+
+    group('allowBackgroundPlayback', () {


Excessive nesting of groups makes IDE UIs harder to use; this can be removed, and the test name below updated to include this string in the name.

stuartmorgan-g · 2022-01-21T18:10:24Z

packages/video_player/video_player_platform_interface/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 5.1.0
+
+* Add `allowBackgroundPlayback` to `VideoPlayerOptions`


Nit: Adds
Nit: Missing period at end of sentence

stuartmorgan-g · 2022-01-21T18:11:25Z

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart


 /// [VideoPlayerOptions] can be optionally used to set additional player settings
 class VideoPlayerOptions {
+  /// Set this to false to let plugin manage play/pause with app lifecycle changes,


Remove this sentence; you don't generally need to document both options for a bool since they are inverse of each other.

stuartmorgan-g · 2022-01-21T18:11:31Z

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart

 /// [VideoPlayerOptions] can be optionally used to set additional player settings
 class VideoPlayerOptions {
+  /// Set this to false to let plugin manage play/pause with app lifecycle changes,
+  /// Set this to true to keep playing video in background, when app goes in background


Nit: missing period.

stuartmorgan-g · 2022-01-21T18:12:16Z

packages/video_player/video_player_web/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.0.6
+
+* Bump `video_player_platform_interface` version


wasn't sure about this change. I had problems with building example without this. But maybe that because there weren't versions constraints when I published pr.

IlyaMax · 2022-01-27T17:37:28Z

@stuartmorgan What do you think about removing allowBackgroundPlayback option from VideoPlayerOptions to simplify this pr? As possible solution I think we can rename VideoPlayerOptions to something like NativeVideoPlayerOptions and make it private for plugin, because allowBackgroundPlayback setting configure dart behavior and create VideoPlayerOptions which duplicates NativeVideoPlayerOptions fields . For example I have idea also to provide option not to recover playing state after AppLifecycleState.resumed which is handled also only on dart side.

stuartmorgan-g · 2022-01-27T17:59:49Z

As possible solution I think we can rename VideoPlayerOptions to something like NativeVideoPlayerOptions

That would be a good option, except that it would be a breaking change for the platform interface package, which we only do when necessary.

We could potentially use the same name and some combination of hide and import as to avoid collisions, but I think that's probably more confusing than just dealing with needing to make Dart-only additions in the platform interface.

IlyaMax · 2022-01-27T18:20:57Z

Yes, it seems that breaking change is not reasonable for now. There are no options to configure on dart apart from these two, at least I can't think of them.

stuartmorgan-g · 2022-01-27T19:36:37Z

(We could theoretically a new secondary options class—e.g., VideoPlayerControllerOptions—and make it an additional optional argument, but that would make the public-facing API really confusing, so isn't something I'd actually want to do.)

stuartmorgan-g · 2022-02-08T18:28:03Z

@gaaclarke Ping on secondary review

gaaclarke

lgtm

gaaclarke · 2022-02-10T01:08:56Z

packages/video_player/video_player/lib/video_player.dart

  Future<void> initialize() async {
-    _lifeCycleObserver = _VideoAppLifeCycleObserver(this);
-    _lifeCycleObserver.initialize();
+    final allowBackgroundPlayback =


nit: The linter isn't flagging this for omitting the datatype?

We haven't converted video_player off of the legacy options yet unfortunately.

gaaclarke · 2022-02-10T01:14:22Z

packages/video_player/video_player/test/video_player_test.dart

 }

 void main() {
+  void verifyAppLifeCycle(


nit: this name isn't very helpful. Something like verifyPlayingWhenAppLifeCyclePaused?

stuartmorgan-g · 2022-02-10T16:58:08Z

@IlyaMax Since we're down to just very minor nits now, please feel free to open the platform interface PR, and we can start getting the pieces of this landed :)

IlyaMax · 2022-02-17T14:30:28Z

@stuartmorgan is it ok if I close this pr and open a new one? I've deleted old repo, so I'm not sure how I can fix nits without opening new pr.

gaaclarke · 2022-02-17T17:24:54Z

@stuartmorgan is it ok if I close this pr and open a new one? I've deleted old repo, so I'm not sure how I can fix nits without opening new pr.

The code still exists on github and you can check it out at surfstudio:add-option-to-not-observe-lifecycle-final-pr.

Some thing like:

git clone [email protected]:flutter/plugins.git
cd plugins
git checkout origin/add-option-to-not-observe-lifecycle-final-pr -b add-option-to-not-observe-lifecycle-final-pr

Alternatively you could try using github's built in editor so you can edit the files without downloading them locally.

IlyaMax · 2022-02-19T15:14:03Z

@gaaclarke this doesn't work unfortunately. This branch has conflicts so it will be hard to manage with this in web editor. In both my new fork and clone this is the list of available branches.

I can't checkout pr branches. Is this so critical if I create a new pr and link this pr there?

stuartmorgan-g · 2022-02-22T15:11:53Z

Given that you've deleted the remote repository, I don't think there's anything you can do other than make a new PR. AFAIK GitHub doesn't allow changing the source of a PR after the fact.

(In the future, please don't delete the repository hosting an active PR. Changing PRs is disruptive to the review process since we can't see incremental diffs and can't easily reference previous comments.)

IlyaMax · 2022-02-22T16:07:21Z

@stuartmorgan Actually I thought that this can be handled with git/github, so didn't think of that. Deleting repos is not something which I do often) but will take this into account next time, yes.

IlyaMax added 2 commits September 8, 2021 20:21

add option to not observe lifecycle

f97c933

remove redundant

1c55067

google-cla bot added the cla: no label Sep 8, 2021

github-actions bot added p: video_player platform-web labels Sep 8, 2021

google-cla bot added cla: yes and removed cla: no labels Sep 8, 2021

IlyaMax changed the title ~~Added new field observeAppLifecycle to enable background playing of video~~ [video_player] Added new field observeAppLifecycle to enable background playing of video Sep 8, 2021

IlyaMax and others added 4 commits September 8, 2021 20:44

merge

2e025ae

Merge branch 'master' into add-option-to-not-observe-lifecycle-final-pr

d29c804

remove redundant version bump

db146f2

fix pubspec

0f89d4f

github-actions bot removed the platform-web label Sep 9, 2021

IlyaMax added 4 commits September 9, 2021 14:52

remove redundant version bump again

4f0c4a1

bump version of video player

e18a461

remove dependency

c24e410

check ci

001065e

github-actions bot added the platform-web label Sep 9, 2021

stuartmorgan-g added the backlog label Sep 9, 2021

stuartmorgan-g removed the backlog label Dec 16, 2021

stuartmorgan-g suggested changes Dec 16, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

IlyaMax added 2 commits January 7, 2022 12:18

update versions to match semver

1d004c5

update versions to match semver

69a6cd5

stuartmorgan-g suggested changes Jan 21, 2022

View reviewed changes

stuartmorgan-g requested review from gaaclarke and removed request for LHLL, bparrishMines, cyanglaz and ditman January 21, 2022 18:13

flutter deleted a comment from ZhangZhiH Jan 21, 2022

IlyaMax added 2 commits January 21, 2022 20:50

remove bump in video_player_web

996ddae

remove dependency override also

f9eaf37

github-actions bot removed the platform-web label Jan 21, 2022

IlyaMax and others added 2 commits January 21, 2022 21:04

fix other nits

55cf52d

Merge branch 'main' into add-option-to-not-observe-lifecycle-final-pr

c941f76

IlyaMax requested a review from stuartmorgan-g January 21, 2022 19:08

gaaclarke approved these changes Feb 10, 2022

View reviewed changes

IlyaMax mentioned this pull request Feb 11, 2022

[video_player] add allowBackgroundPlayback option platform interface changes #4807

Merged

11 tasks

IlyaMax mentioned this pull request Feb 23, 2022

[video_player] Added new option allowBackgroundPlayback #4908

Closed

11 tasks

IlyaMax closed this Feb 23, 2022

		@@ -1,3 +1,7 @@
		## 2.3.0

		* Add `allowBackgroundPlayback` to `VideoPlayerOptions`

		@@ -1,3 +1,7 @@
		## 5.1.0

		* Add `allowBackgroundPlayback` to `VideoPlayerOptions`

		@@ -1,3 +1,7 @@
		## 2.0.6

		* Bump `video_player_platform_interface` version

[video_player] Added new field allowBackgroundPlayback to enable background playing of video #4324

[video_player] Added new field allowBackgroundPlayback to enable background playing of video #4324

Uh oh!

Conversation

IlyaMax commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

google-cla bot commented Sep 8, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

IlyaMax commented Sep 8, 2021

Uh oh!

stuartmorgan-g commented Sep 9, 2021

Uh oh!

ZhangZhiH commented Sep 28, 2021

Uh oh!

IlyaMax commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZhangZhiH commented Oct 7, 2021

Uh oh!

ZhangZhiH commented Oct 10, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

blessedman commented Dec 18, 2021

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IlyaMax commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 27, 2022

Uh oh!

IlyaMax commented Jan 27, 2022

Uh oh!

stuartmorgan-g commented Jan 27, 2022

Uh oh!

stuartmorgan-g commented Feb 8, 2022

Uh oh!

IlyaMax commented Sep 8, 2021 •

edited

Loading

IlyaMax commented Oct 4, 2021 •

edited

Loading

IlyaMax commented Jan 27, 2022 •

edited

Loading

IlyaMax commented Feb 19, 2022 •

edited

Loading