Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit system video view Picture in Picture support #1015

Closed
8 tasks done
defagos opened this issue Sep 24, 2024 · 1 comment · Fixed by #1017
Closed
8 tasks done

Revisit system video view Picture in Picture support #1015

defagos opened this issue Sep 24, 2024 · 1 comment · Fixed by #1017
Assignees
Labels
bug Something isn't working

Comments

@defagos
Copy link
Member

defagos commented Sep 24, 2024

As an app developer I want to be able to use Picture in Picture with any layout configuration, as for VideoView, without it being interrupted and without any video layer being lost.

Acceptance criteria

  • The VideoView PiP demos are provided for SystemVideoView as well.
  • All possible use cases work equally well with SystemVideoView.

Tasks

  • Update PiP-related demos with corresponding system video view entries.
  • Adopt similar strategy as VideoView to fix issues.
  • Go through all PiP use cases (see Support advanced PiP integration #612) and ensure nothing is broken.
  • Check memory profile.
  • Check behavior on tvOS 16, 17 and 18.
  • Check behavior on iOS 16, 17 and 18.
  • Improve documentation if needed (some restrictions are already documented in the PiP article).
  • Document that auto-switch to PiP does not work when multiple PiP-supporting system video views are displayed at the same time.
@defagos defagos converted this from a draft issue Sep 24, 2024
@defagos defagos added the bug Something isn't working label Sep 24, 2024
@waliid waliid linked a pull request Sep 24, 2024 that will close this issue
4 tasks
@defagos
Copy link
Member Author

defagos commented Sep 26, 2024

A few remarks:

  • We previously did not implement PiP dismissal for our inline system video view demo. Dismissal is in fact possible but should occur after PiP started, otherwise the system might pause playback for some reason. We have updated our documentation and demos accordingly.
  • During our implementation we wanted to add a Player parameter to PiP delegate / lifecycle methods. When several PiP-enabled views are displayed at the same time, the implementation namely doesn't know which player PiP is started for. We thus thought this additional piece of information could be helpful (e.g. so that implementations can pause other players), but discovered during implementation that this was adding a lot of unnecessary complexity in our implementation, especially related to the lifecycle of involved objects. Not adding a player parameter to PiP delegate / lifecycle methods should not be a problem, though, as PiP should really be enabled for one view at a time, which means an implementation must know the current player identity already.
  • We discovered that setting the contentSource of a PiP controller does not properly cleanup all associated resources, as could be naively expected. To solve this issue it suffices to assign an empty source (content source built from a dummy layer) instead.
  • We tried an alternative implementation to avoid PiP issues when started from full-screen system video view presentation (avoiding the use of viewWillAppear(_:) and associated presentation information) but this was not working in edge cases (e.g. when replacing the player view in the SRF Sports app with a SystemVideoView). The original approach was therefore preserved. Because of new view controller hosting we had to introduce, though, presentation information must be obtained by navigating the view controller hierarchy upwards, as is done in Play SRG.

@github-project-automation github-project-automation bot moved this from 🚧 In Progress to ✅ Done in Pillarbox Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants