packages icon indicating copy to clipboard operation
packages copied to clipboard

[video_player] Implements background playback functionality

Open ArvidNy opened this issue 8 months ago • 22 comments

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 https://github.com/flutter/flutter/issues/62739

Pre-Review Checklist

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

[^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.

ArvidNy avatar May 06 '25 12:05 ArvidNy

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.

stuartmorgan-g avatar May 06 '25 19:05 stuartmorgan-g

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?

ArvidNy avatar May 07 '25 11:05 ArvidNy

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

bparrishMines avatar May 07 '25 19:05 bparrishMines

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

stuartmorgan-g avatar May 07 '25 19:05 stuartmorgan-g

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 avatar May 07 '25 20:05 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 avatar May 07 '25 20:05 ArvidNy

I see the issue; I've filed https://github.com/flutter/flutter/issues/168538 to track it.

The best solution in the context of this PR is to revert all the non-video_player* package changes.

stuartmorgan-g avatar May 08 '25 12:05 stuartmorgan-g

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?

ArvidNy avatar May 09 '25 09:05 ArvidNy

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.

stuartmorgan-g avatar May 09 '25 12:05 stuartmorgan-g

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.

ArvidNy avatar May 16 '25 14:05 ArvidNy

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 https://github.com/flutter/packages/pull/9316 (and looking at the Android changes here, they were not safe without that change, because they would cause racy crashes).

stuartmorgan-g avatar May 27 '25 13:05 stuartmorgan-g

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.

ArvidNy avatar May 27 '25 15:05 ArvidNy

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?

stuartmorgan-g avatar May 27 '25 16:05 stuartmorgan-g

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: https://github.com/flutter/packages/blob/cca0a59e9b0a7cf086b92aa23023fa19d24a44aa/packages/video_player/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java#L61-L62

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?

ArvidNy avatar May 27 '25 21:05 ArvidNy

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.

stuartmorgan-g avatar Jun 03 '25 15:06 stuartmorgan-g

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.

ArvidNy avatar Jun 04 '25 11:06 ArvidNy

Why this is not yet merged?

dejancencelj avatar Aug 07 '25 12:08 dejancencelj

@ArvidNy Are you still planning to address the remaining review feedback?

stuartmorgan-g avatar Aug 19 '25 18:08 stuartmorgan-g

Sure, I plan on resolving the feedback I got so this can be merged. I've been busy with other projects for the past months, but I hope I'll get around to it soon.

ArvidNy avatar Aug 20 '25 20:08 ArvidNy

I've now created a PR for only the platform interface package changes: #10054. I made the changes that the Gemini bot suggested if you want to review the new PR now.

ArvidNy avatar Sep 22 '25 15:09 ArvidNy

Now that the interface PR landed, can you resolve the conflicts and let us know when it is ready for re-review?

mboetger avatar Oct 14 '25 21:10 mboetger

OK, I think it's finally ready for a re-review now.

ArvidNy avatar Oct 20 '25 22:10 ArvidNy