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

Make Pillarbox thread-safe #931

Open
13 tasks
defagos opened this issue Jul 7, 2024 · 0 comments
Open
13 tasks

Make Pillarbox thread-safe #931

defagos opened this issue Jul 7, 2024 · 0 comments
Labels
enhancement New feature or request

Comments

@defagos
Copy link
Member

defagos commented Jul 7, 2024

As a developer integrating Pillarbox I want to be able to use and call Pillarbox player APIs from any thread.

Acceptance criteria

  • Pillarbox APIs have been made thread-safe where appropriate.

Hints

Currently Pillarbox is implemented with main-thread usage in mind. Most of updates are made on the main thread (e.g. publishers always send back their output on the main thread before properties they are connected to are updated) and the ObservableObject nature of UI-related classes makes this choice obvious.

Since iOS and tvOS 16, though, AVPlayer APIs can be called from any thread. The current behavior therefore adds a limitation on top of an API that does not require it.

Actors are no solution to this issue since they make all access async, which is in our case not ergonomic.

To help us in implementing thread-safety we must also answer the question: On which thread is a value sent to a publisher on some thread received? On the same one? Always?

Tasks

  • Make the move to @Observable. The advantage is that observed changes will be automatically fetched on the main thread by SwiftUI. Of course this does not make anything more thread-safe, but if users continue to use our APIs on the main thread this should not be a problem.
  • Implement thread-safety strategies. We can likely use locks to achieve our goal since the APIs are not meant to be called often. Other approaches are likely overkill and this article nicely sums up possibilities we have.
    • Each property needs to be protected separately (same lock) if we want to assign them with Combine I guess. Maybe having a macro could be helpful to avoid having each property backed by another private one just for the sake of protecting its get / set with a lock.
    • Have publishers deliver results on a common background queue.
    • Where appropriate we should of course use a lock to protect sections of code.
    • Have tracker APIs be called on background threads and documented as such.
    • Where AVPlayer APIs are used there might be no need for additional protection.
  • Mark Player as @unchecked Sendable.
  • Mark the APIs which are really UI-related (ProgressTracker, VisibilityTracker, LayoutReader) with @MainActor. Keep others non-annotated (Player, tracker APIs).
  • Update documentation:
    • Publishers delivering values on background threads.
    • Player item tracker methods called on background queue (or anywhere in the case of a deinit that calls a delegate method).
    • Player can be used from any thread.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 📋 Backlog
Development

No branches or pull requests

1 participant