[video_player] Implements background playback functionality

[ArvidNy]

This pull request adds the missing implementation for background playback. This allows video_player to be used with e.g. audio_service to continue playing audio after the screen is closed or the app is move to the background. A sample implementation for testing can be found here: https://github.com/ArvidNy/video_player_audio_service

Closes flutter/flutter#62739

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[stuartmorgan-g]

Thanks for the contribution! Please see https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#changing-federated-plugins for the process to make a multi-package PR, so that our CI can run tests.

[ArvidNy]

Thanks for the link, I missed that!

I might be missing something very obvious here, but I'm not sure how to update so that the remaining checks will complete. From what I understand it fails due to a cached version of video_player_platform_interface instead of using the path-based dependency:

 Compilation failed for testPath=/b/s/w/ir/x/w/packages/packages/camera/camera/example/test/main_test.dart: ../../../video_player/video_player/lib/video_player.dart:440:34: Error: The method 'setAllowBackgroundPlayback' isn't defined for the class 'VideoPlayerPlatform'.
   - 'VideoPlayerPlatform' is from 'package:video_player_platform_interface/video_player_platform_interface.dart' ('../../../../../.pub-cache/hosted/pub.dev/video_player_platform_interface-6.3.0/lib/video_player_platform_interface.dart').
  Try correcting the name to the name of an existing method, or defining a method named 'setAllowBackgroundPlayback'.
        await _videoPlayerPlatform.setAllowBackgroundPlayback(

I can get the camera example to run locally, but only if I use direct dependencies instead of overridden dependencies.

What's the best approach here? Should I just remove the path-based dependencies for the camera example or is there another way to resolve it?

[bparrishMines]

@stuartmorgan-g It looks like dependency_overrides doesn't work for transitive dependencies: https://dart.dev/tools/pub/dependencies#dependency-overrides

Only the dependency overrides in a package's own pubspec are considered during package resolution. Dependency overrides inside any depended-on packages are ignored.

@ArvidNy You should be able to fix this by manually adding the rest of the dependencies to the failing plugins. They should look like:

dependency_overrides:
  video_player: {path: ../../../../packages/video_player/video_player}
  video_player_android: {path: ../../../../packages/video_player/video_player_android}
  video_player_avfoundation: {path: ../../../../packages/video_player/video_player_avfoundation}
  video_player_platform_interface: {path: ../../../../packages/video_player/video_player_platform_interface}
  video_player_web: {path: ../../../../packages/video_player/video_player_web}

Let me know if that works.

[stuartmorgan-g]

@stuartmorgan-g It looks like dependency_overrides doesn't work for transitive dependencies

Yes, the tooling should account for that though.

@ArvidNy You should be able to fix this by manually adding the rest of the dependencies to the failing plugins.

That shouldn't have been necessary. What command did you run, exactly, to generate what's in the PR now?

[ArvidNy]

This is the command I ran:

 dart run script/tool/bin/flutter_plugin_tools.dart make-deps-path-based --target-dependencies=video_player_platform_interface,video_player_android,video_player_avfoundation,video_player,video_player_web

[ArvidNy]

Oh, and manually adding the rest of the dependency overrides does seem to resolve the issue, at least locally. So I can push that to resolve the PR tests if there's no other obvious fix for it.

[ArvidNy]

I have reverted all the changes in the example files in the other packages, but for some reason the tests still fail with those packages from what I can see. Not sure what more to do. Any suggestions or is it possible to review anyway?

[stuartmorgan-g]

Ah yes, the pathified tests hit the same issue.

We can ignore those failures for review; they won't affect the ability to ultimately land the PR since once sub-PRs are split out and landed it won't happen any more.

[ArvidNy]

Just to ensure I've understood the process correctly, I'm awaiting the actual review now before I proceed with the other PRs, right? Just want to make sure that you're not waiting for me to do anything more at the moment.

[stuartmorgan-g]

From an initial skim of this PR, it appears that it is plumbing no-ops all the way through to the native layer on most platforms; it's not clear what that is intended to accomplish.

Is this functionally intended to be Android-only? If so, it is likely obsoleted by #9316 (and looking at the Android changes here, they were not safe without that change, because they would cause racy crashes).

[ArvidNy]

I figured that it would be better to allow this option to be passed to the native layer the same way as mixWithOthers, so I looked at that implementation and made a similar approach here. But yes, the real change here is for Android.

I'll test the new changes in that PR and see if that resolves this issue or if the wake lock part also is needed for background playback.

[stuartmorgan-g]

I figured that it would be better to allow this option to be passed to the native layer

I'm not sure I follow. Why is adding more code complexity and more runtime overhead to no-op on the native side better than no-op'ing immediately in Dart?

[ArvidNy]

I figured you would want consistency in the code. On Android we don't always want to use wake lock so we need to know when the option is true on the Flutter side. The option allowBackgroundPlayback was missing on the Java end, so I traced how mixWithOthers was set and used the same format to be able to use it like this:

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

Lines 61 to 62 in cca0a59

	if (options.allowBackgroundPlayback) {
	exoPlayer.setWakeMode(C.WAKE_MODE_NETWORK);

I'm not sure that's better, but it allows for flexibility in case Apple would change anything in the future and we need to know when the option is set on another platform as well.

That being said, I'm all for reducing the complexity of the PR if that's preferable. Is it better to only add the method to the Android implementation then and not the VideoPlayerPlatform abstract class to avoid having to override it on all platforms?

[stuartmorgan-g]

I figured you would want consistency in the code.

We want each implementation to be written in the way that makes sense for that implementation.

I'm not sure that's better, but it allows for flexibility in case Apple would change anything in the future and we need to know when the option is set on another platform as well.

The Dart code and native code in question are part of the same package. There is no case where have the ability to change the native code in a package but not the Dart code in the same package, so I'm not sure what flexibility you are referring to.

That being said, I'm all for reducing the complexity of the PR if that's preferable. Is it better to only add the method to the Android implementation then and not the VideoPlayerPlatform abstract class to avoid having to override it on all platforms?

Making a no-op implementation in Dart, avoiding unnecessary round trips to the host side, does not require removing the cross-platform abstraction. @override Future<void> setAllowBackgroundPlayback(bool allowBackgroundPlayback) async {} is valid code that has the same effect as what the PR currently does on iOS, but with far less overhead.

What the cross-platform abstraction should or shouldn't be is a separate question of behavior, not implementation. That's a question of what happens on other platforms. For instance, what is the expected behavior of setAllowBackgroundPlayback(true) and setAllowBackgroundPlayback(false) on iOS? Currently this PR doesn't do the right things for at least one of those, it seems. See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#platform-support for discussion of that aspect.

[ArvidNy]

Oh, you're right. I didn't think about that fact that the communication with the host side was unnecessary there and I didn't make that connection from your previous message – sorry about that. I've just removed the no-op on the native side for iOS and merged the latest changes from the PR you mentioned.

For Android, the setWakeMode still needs to be called, so the other PR only partly resolved the background playback issue.

[harryterkelsen]

Web implementation (no-op that instantly returns) looks good to me