[video_player]: Support for optional bypassing of audio setup

[mitghi]

trafficstars

This PR adds support for bypassing the audio setup. On iOS, the platform initialize method reconfigures the global audio setup ( forcefully ) each time the plugin instance gets created or after each restart.

The support is also added to configure audio session in future versions via a message.

bypassAudioSetup option is added to VideoPlayerOptions. When set to true, it prevents any changes to audio session, essentially bypassing it. By default, it is set to false and the behaviour is exactly production version

VideoPlayerOptions({bypassAudioSetup: true})

This PR fixes issue #94328.

Pre-launch Checklist

• [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• [x] I read and followed the [relevant style guides] and ran [the auto-formatter]. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• [x] I signed the [CLA].
• [x] The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• [x] I listed at least one issue that this PR fixes in the description above.
• [x] I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
• [x] I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
• [x] I updated/added relevant documentation (doc comments with ///).
• [x] I added new tests to check the change I am making.
• [x] All existing and new tests are passing.

[google-cla[bot]]

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

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[mitghi]

@stuartmorgan @jmagman I have recreated the changes on top of federated video_player.

[mitghi]

The web changes (a noop) are OK by me. Do mention me when the time comes to land the web package PR! The rest of the platform owners should take a look at this too.

Thank you for your feedback. Sure, I will.

[gaaclarke]

Don't you think it makes more sense to create a plugin that sets the audio mixing mode for the app instead of tying the functionality to the video player? The way it works on iOS is that it is a global variable so setting it implicitly in a plugin probably isn't the best choice. If it is it's own plugin it can be used in conjunction with other plugins too instead of each plugin implementing this feature and having them fight each other about what the behavior should be.

[mitghi]

Don't you think it makes more sense to create a plugin that sets the audio mixing mode for the app instead of tying the functionality to the video player? The way it works on iOS is that it is a global variable so setting it implicitly in a plugin probably isn't the best choice. If it is it's own plugin it can be used in conjunction with other plugins too instead of each plugin implementing this feature and having them fight each other about what the behavior should be.

Thank you for the feedback. It does make sense. The problem that I had encountered is that the AVAudioSession will get forcefully reconfigured by video player during execution of initialize method. This happens each time an instance gets created. Therefore, it resets any configuration applied to AVAudioSession. What would be your suggestion to overcome this issue?

https://github.com/flutter/plugins/blob/5ed15e5ceba01f32422d0941f0b7f8ac5a92d24c/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m#L533

This PR extends the functionality similar to setMixWithOthers(...).

It would be great to decouple setup of AVAudioSession from video player when possible. Because otherwise it creates unwanted hiccups and unexpected behavior.

[gaaclarke]

Yea, seems like we'll have to do a breaking change. What I suggest is removing the setCategory call from the initialize message and adding a separate call to the plugin that sets the mixing mode. While I think a separate plugin makes more sense, it's kind of a bad breaking change if the fix is you have to bring in a new plugin. Then in the breaking change description they'll have everything they need to duplicate the old behavior, they just have to add an extra call to set the mixing mode. How's that sound @stuartmorgan ?

[mitghi]

Yea, seems like we'll have to do a breaking change. What I suggest is removing the setCategory call from the initialize message and adding a separate call to the plugin that sets the mixing mode. While I think a separate plugin makes more sense, it's kind of a bad breaking change if the fix is you have to bring in a new plugin. Then in the breaking change description they'll have everything they need to duplicate the old behavior, they just have to add an extra call to set the mixing mode. How's that sound @stuartmorgan ?

I am also in favor of this solution, to add a separate method to setup the audio session or skipping it completely.

[stuartmorgan-g]

I'm not sure we actually need a breaking change. It seems like we could:

1. Separate that call out of initialize on the native side (which needs to be done either way), adding a call to the new method in the iOS Dart init (thus not a breaking change)
2. Add a new platform interface method that takes an audio settings parameter object (which needs to be done either way), but also add a new variant of init that takes a parameter for making global changes automatically (not a breaking change; see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-platform-interface-method-parameters).
3. Implement the new methods in the iOS implementation; in the new init, only call the platform method for audio settings if requested.
4. Update the app-facing package to use the new init, and then optionally call the other new method based on a new optional constructor argument.

[gaaclarke]

I'm not sure we actually need a breaking change. It seems like we could:

I'm suggesting we make it a breaking change because the current default behavior is wrong (ie the plugin is implicitly mutating global state) and we should change that. If we do that though, that means the mixing behavior that some apps rely on would change. So, breaking in a semantics sense, not an API sense.

I think it makes the most sense to completely remove the mutations of the global state from the plugin, but for the sake of ease of transition I'm fine adding it in somewhere. We just can't make it something that happens without explicitly asking for it.

[stuartmorgan-g]

I'm suggesting we make it a breaking change because the current default behavior is wrong (ie the plugin is implicitly mutating global state) and we should change that.

At some point, yes. But there are about 150 public packages that depend on video_player, so the ecosystem effects of a breaking change are non-trivial. This is fixing a niche edge case (the issue it fixes has zero votes currently) on one platform, while the incompatibilities as a major version change works its way through the ecosystem will affect many more people.

I'm all for filing an issue to batch changing the default into the next breaking change, but we shouldn't do a breaking change solely for this issue.

[mitghi]

@stuartmorgan If I understand it correctly, this call then, will not result in global configuration of AVAudioSession:

https://github.com/flutter/plugins/blob/main/packages/video_player/video_player/lib/video_player.dart#L28

and based on the given options, the correct platform methods could be called from plugin initialize method:

https://github.com/flutter/plugins/blob/main/packages/video_player/video_player/lib/video_player.dart#L318

How about the options specific to iOS ( like category, session mode and etc )? When it get added to VideoPlayerOptions, should it get discarded by Android implementation? Or is there a way to make it iOS specific? ( In case we implement a method to allow the client to have full control over AVAudioSession )

Thanks

EDIT: I mean something similar to this in video_player_platform_interface.dart:

enum AudioSessionCategory {
  none,
  ambient,
  multiRoute,
  playAndRecord,
  playback,
  record,
  soloAmbient,
}

enum AudioSessionCategoryOption {
  none,
  mixWithOthers,
  duckOthers,
  interruptSpokenAudioAndMixWithOthers,
  allowBluetooth,
  allowBluetoothA2DP,
  allowAirPlay,
  defaultToSpeaker,
  overrideMutedMicrophoneInterruption,
}

enum AudioSessionMode {
  none,
  modeDefault,
  gameChat,
  measurement,
  moviePlayback,
  spokenAudio,
  videoChat,
  videoRecording,
  voiceChat,
  voicePrompt,
}

@immutable
class AudioSessionOption {
  AudioSessionOption({
      this.category = AudioSessionCategory.none,
      this.categoryOption = AudioSessionCategoryOption.none,
      this.mode = AudioSessionMode.none,
      this.bypassSession = false,
  });

  final AudioSessionCategory category;
  final AudioSessionCategoryOption categoryOption;
  final AudioSessionMode mode;
  final bool bypassSession;
}


/// [VideoPlayerOptions] can be optionally used to set additional player settings
@immutable
class VideoPlayerOptions {
  /// set additional optional player settings
  // TODO(stuartmorgan): Temporarily suppress warnings about not using const
  // in all of the other video player packages, fix this, and then update
  // the other packages to use const.
  // ignore: prefer_const_constructors_in_immutables
  VideoPlayerOptions({
    this.mixWithOthers = false,
    this.allowBackgroundPlayback = false,
    this.audioSessionOption = AudioSessionOption(),
  });

  /// Set this to true to keep playing video in background, when app goes in background.
  /// The default value is false.
  final bool allowBackgroundPlayback;

  /// Set this to true to mix the video players audio with other audio sources.
  /// The default value is false
  ///
  /// Note: This option will be silently ignored in the web platform (there is
  /// currently no way to implement this feature in this platform).
  final bool mixWithOthers;

  final AudioSessionOption audioSessionOption;
}

[mitghi]

@gaaclarke I think the second option you have suggested might work better in this case, to bypass the configuration of AVAudioSession and delegate the setup to another plugin only when needed? otherwise fallback to default behaviour.

@immutable
class VideoPlayerOptions {
  ...

  VideoPlayerOptions({
    this.mixWithOthers = false,
    this.allowBackgroundPlayback = false,
    this.bypassAudioSession = false,
  });

    ...
    final bool bypassAudioSession;
}

[stuartmorgan-g]

How about the options specific to iOS ( like category, session mode and etc )? When it get added to VideoPlayerOptions, should it get discarded by Android implementation? Or is there a way to make it iOS specific? ( In case we implement a method to allow the client to have full control over AVAudioSession )

If we were to add extremely iOS(+macOS)-specific options to this plugin, we would do so in video_player_avfoundation, not in the app-facing package. Only settings generic enough to plausibly be multi-platform (which stops being the case once you are just duplicating enums from one platform's APIs) would go in a cross-platform API. But if you want to expose the ability to control the entire suite of exact AVAudioSession global options, that should be a new plugin, not any part of video_player.

I strongly recommend that we focus this PR on making the global state mutation optional instead of forced (as described in my comment above), and then discuss what new options, if any, should be added to video_player in a new issue and/or design doc.

[mitghi]

@stuartmorgan Thank you for the feedback. Alright, I will do so as recommended.

[mitghi]

@stuartmorgan I have reimplemented the changes as suggested.

[cyanglaz]

@stuartmorgan friendly ping.

[hellohuanlin]

Any updates on this one? @mitghi

[hellohuanlin]

Any updates on this one? @mitghi

Sorry I saw you updated the PR. I will take another look.

[mitghi]

@stuartmorgan Thank you for the feedback. I make the changes.

[cyanglaz]

@mitghi Friendly ping. Are you planning to resolve the comments from Stuart?

[mitghi]

@cyanglaz Yes, I am waiting for @stuartmorgan to clarify some of my questions in order to proceed.

[stuartmorgan-g]

@mitghi What questions are you referring to? I don't see anything since my last round of feedback.

[mitghi]

@stuartmorgan I left a comment in the review section under your message about breaking change ( removing AVAudioSession call from iOS initialize method ). I copy it here:

Could you please provide me with an example? What happens to instances which are created by the getters in platform api? in those getters no option could be supplied and for me its unclear how the new init function can help there. Because the instance it return are always bounded to current init function, thus modifying global audio session. I need a bit of help to comprehend this part, the rest is clear. Should the getters change as well?

I did not thought about the federation, you are absolutely right.

Thanks

EDIT: If i understand it correct, a new initialize method which is almost identical with current implementation except it does not modify global state?

https://github.com/flutter/plugins/blob/main/packages/video_player/video_player/lib/video_player.dart#L23

VideoPlayerPlatform get _videoPlayerPlatform {
  final VideoPlayerPlatform currentInstance = VideoPlayerPlatform.instance;
  if (_lastVideoPlayerPlatform != currentInstance) {
    // This will clear all open videos on the platform when a full restart is
    // performed.
    currentInstance.init();
    _lastVideoPlayerPlatform = currentInstance;
  }
  return currentInstance;
}

[stuartmorgan-g]

@stuartmorgan I left a comment in the review section under your message about breaking change

I can't see that anywhere; perhaps you forgot to publish it and it's still "pending"?

If i understand it correct, a new initialize method which is almost identical with current implementation except it does not modify global state?

Yes, that's correct.

What happens to instances which are created by the getters in platform api? in those getters no option could be supplied and for me its unclear how the new init function can help there.

Once the new methods (new init, new method for setting global state) exist in the platform interface, the app-facing package will need to be adjusted to (conditionally) call the new method before anything else, and to use the new init.

[stuartmorgan-g]

@mitghi Are you still planning on updating this per the last comments about changing the init flow?

[stuartmorgan-g]

I'm going to mark this as a draft for now since there are outstanding comments, just to get this off our PR review queue. Once you've had time to address the comments above please mark it as ready for review again. Thanks!

[Hixie]

@mitghi Thanks for your contribution; is this PR still on your radar?

[stuartmorgan-g]

Since this is marked as a draft and hasn't been updated in several months I'm going to close it to clean out our review queue. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!