Picture in Picture is killed too early if no `VideoView` is initially present

[defagos]

Description of the problem

The current reference counting strategy implemented for PiP sadly has a flaw. We did not notice it because of our demo implementation but in general it will fail if the player view enabled for PiP is initially displayed without VideoView actually in the hierarchy.

In this case the reference counter is not increased initially on view entry (since playerLayer is nil) but will be on exit, leading to early PiP controller deallocation.

Relevant stack trace or log output

No response

Reproducibility

Always

Steps to reproduce

1. Edit the PlaybackView code so that no VideoView is displayed when the stream type is unknown (always the case initially in our demo):

   @ViewBuilder
   private func video() -> some View {
       switch player.mediaType {
       case .audio:
           image(name: "music.note.tv.fill")
       case .video:
           if player.isExternalPlaybackActive {
               image(name: "tv")
           }
           else {
               VideoView(player: player, gravity: gravity, isPictureInPictureSupported: isPictureInPictureSupported)
           }
       case .unknown:
           Color.clear
       }
   }

2. Run the demo, open an example with the custom player and enable PiP with the dedicated button. PiP is incorrectly exited.

Library version

0.8.0

Operating system

iOS 17

Code sample

No response

Is there an existing issue for this?

• [X] I have searched existing issues and found no similar bug report.

[defagos]

I already opened a branch and updated the demo according to the above report. I have no clue for a solution yet.

[defagos]

I think I found a strategy:

• Turn enabledForInAppPictureInPictureWithCleanup(perform:) into methods on VideoView and SystemVideoView. This way we cannot have a dangling modifier without associated view. This should fix the reference counting issue reliably.
• Update our implementation so that it compiles again.
• Maybe our implementation can then be simplified. We namely might be able to get rid of the PictureInPicture umbrella object in the process (remains to be investigated).

If this strategy works there is also probably an opportunity to change the API so that the basic and in-app PiP are two different modifiers (or parameters to video views), rather than having a Boolean and an associated closure on a modifier (leading to combinations that do not make sense, e.g. PiP not supported but cleanup block assigned).

It might even be possible to make our implementation simpler.

All use cases must pass as before, e.g.:

• Simple restoration.
• Restoration to the same content.
• Restoration to another video content.
• Restoration to another audio content.
• Restoration to a failing content.
• System PiP with full screen integration.
• System PiP with inline integration.
• ...

[defagos]

Not sure the above strategy works, as it binds the lifetime of the extended PiP to the lifetime of the video view, thus extended nothing.

Working on another strategy. Just a side note: It seems pauses during PiP restoration might occur when the item is switched before the PiP restoration. The item should really be updated after PiP will stop delegate method has been called.

[defagos]

The very same issue affects our ability to implement advanced PiP in the MultiView example.

[defagos]

The issue has been fixed in PR #702. The demo has also been improved to test advanced PiP in the context of multiple players and multiple views.