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

Publish consolidated item context #559

Closed
3 tasks done
defagos opened this issue Sep 5, 2023 · 6 comments · Fixed by #587
Closed
3 tasks done

Publish consolidated item context #559

defagos opened this issue Sep 5, 2023 · 6 comments · Fixed by #587
Assignees
Labels
enhancement New feature or request

Comments

@defagos
Copy link
Member

defagos commented Sep 5, 2023

As a Pillarbox developer I want to build complex behaviors on consistent state. Currently essential item-related information (status, seekable ranges, media selection, etc.) stems from various asynchronous sources which might not deliver the same information at the same time, which can lead to subtle synchronization issues.

Context

When implementing track analytics (#508) we discovered that the initial play event has undefined audio and subtitle selection information. While this is not an issue according to our specifications this is still puzzling.

The issue is related to the fact that our tracker subscribes to various sources of change, several of which are separately dependent on the item status. In many cases relevant information like the current media selection, for example, is only delivered once the item is ready to play. The item state publisher and these other publishers, all dependent on the item status, deliver their changes at different times (almost at the same time), leading to an inconsistent picture.

Thus we should attempt to:

  • Group item-related properties that depend on the status in a common context struct (or similar).
  • Deliver all these changes via the same pipeline, ensuring they are all updated in sync, especially when playback starts.
  • Subscribe to relevant publishers so that the context is later updated, e.g. when the seekable range or the media selection change.

This should have no negative impact on our asynchronous / reactive approach but would increase state coherence. It would likely be a bit more challenging to test context updates globally but we can likely still test individual sources of change.

If we can rewrite our implementation with this strategy initial play events should readily have correct media selection information.

Acceptance criteria

  • The above strategy has been investigated (and implemented if possible).
  • The API remains simple to use.
  • No performance issue is added because of this change.

Tasks

  • Identify related item state information which depends on the status being ready to play.
  • Implement single item context delivery pipeline.
  • Hook to a player current item context published property and write existing published properties, not connectable anymore, as computed properties.
@defagos defagos added this to Pillarbox Sep 5, 2023
@defagos defagos converted this from a draft issue Sep 5, 2023
@defagos defagos moved this from ✏️ Draft to 📋 Backlog in Pillarbox Sep 5, 2023
@defagos defagos added the enhancement New feature or request label Sep 5, 2023
@defagos
Copy link
Member Author

defagos commented Sep 5, 2023

Here are the item properties that currently require the item to be ready to play before being meaningful:

  • time range (loaded + seekable)
  • duration
  • media selection
  • presentation size

In fact these are all properties of interest for an item, it is therefore likely we should consolidate everything item-related.

@defagos
Copy link
Member Author

defagos commented Sep 5, 2023

I implemented a PoC with hierarchical state management on 559-publish-consolidated-item-context-poc. This PoC works really well and fixes any kind of possible mismatches between pipelines, as all the state is consolidated into one. Some remarks:

  • All the state is consolidated into a context (we can likely find a better name). We can publish this context, letting any kind of implementation subscribe to internal player updates with a single point of contact. This should make implementing custom trackers a breeze, or make it possible to easily implement a reactive UI without SwiftUI.
  • The loaded and seekable time ranges should likely be loaded separately to keep updates minimal in the main state pipeline (~10 updates).
  • This approach does not fix issues with media selection, though. We need to wait until the item is ready to play before we retrieve the media selection, which means that the initial playing state is notified before the selection has a chance to be loaded. But this approach should make it easy to fix this issue (if this is an issue) in the tracker directly.

Properly implementing this architecture is quite some work, though. We should:

  • Avoid touching the existing code and introduce all hierarchical structures for storing state, migrating tests which make sense one by one. Some publishers to test will require the item to be ready to play so they should wait on this item state first before testing the values they deliver, unlike today where the item state is embedded in the publisher directly. Assertions should be made on publishers which require this state first. All publishers we currently have can be written as convenience publishers extracting streams from the context, e.g. $context.map(\.playbackState).removeDuplicates() for a playback state publisher, where playbackState is a computed property based on rate and current item state. Maybe these publishers could be provided as a convenience API as well, otherwise each publisher can be defined locally in each test.
  • The code must compile at all times during construction of the new toolset required.
  • Each piece of data should be carefully considered:
    • At which level it must reside.
    • Which ones should be involved (probably almost all except time ranges). It is even likely that we do not need time ranges to publish values we store. We can probably have seekable time ranges trigger an update (the real value is then read directly from the player to avoid mismatches with the reality) and the loadable ones are probably only useful for buffering display, so maybe even not in the player directly.
    • Stored items should likely be still published separately. The consolidated state is entirely based on AVQueuePlayer observation, and the queue player itself is kept synchronized with the stored items in a unidirectional way.
  • For each pipeline ensure that a value is delivered immediately, built from the current context (e.g. extracting the current player rate, not hardcoding 1). For pipelines for which no current value can be immediately read (e.g. asset properties) we need to prepend a placeholder value manually first.
  • Once all tools are ready and tested we can hook the context delivery to a player published property. We can then replace prior published properties one by one with computed properties and remove old code in the process (most notably publishers).
  • Improve tracker API contract #560 is then possible. Either we recognize that the $state stream is the key and provide an update method in the tracker contract, with the current item registering to the publisher and providing the context to the method, or we let implementations register like today (in which case publisher helpers we might provide to extract only specific information might be useful, with the risk that some implementations extract several streams and combine them unnecessarily, leading to similar inconsistencies as today).

The robustness achieved with this approach should really benefit our player, I think the result would be worth the investment.

In effect this is like having our player be a simple collection of stored properties accessing its internal AVQueuePlayer state. This way we can automatically subscribe to changes or get a reactive stream of changes reliably. We can see this behavior as having the player publish itself when it changes so that we can get the values just before they are applied, eliminating the tricks we sometimes need to observe after objectWillChange.

A final advantage of this approach is expressiveness. We only need to add a new source of change to the context structure when needed (only low-frequency sources, no time-related ones), then add computed properties to extract interesting consolidated information (e.g. playback state), the implementation being expressive about what is combined. In a sense this is a single source of truth approach where the source of truth is built by a context publisher and valuable consistent information can be extracted from it at any time.

TL;DR: We need to:

  • Consolidate player- and item-related properties into a single publisher, hooked to a published property.
  • Consolidate time- and range-related properties into a time publisher, not hooked to any published property. Maybe we could provide smooth and non-smooth time support with a flag passed when getting the publisher.
  • We can keep our current stored item publisher separate (and hooked to a published property).

@defagos defagos moved this from 📋 Backlog to 🚧 In Progress in Pillarbox Sep 20, 2023
@defagos
Copy link
Member Author

defagos commented Sep 22, 2023

Things to do:

  • Control center.
  • Trackers.
  • Ensure correct behavior when restarting after item failure (restart must be possible).
  • Remove duplicates consistently for each pipeline.
  • Find proper name for context (maybe Properties instead).
  • Add helpers to access context information? (e.g. isBuffering)
  • Mark API as public.
  • Cleanup imports.
  • Check each and every unit test. Test item publisher and item transitions in a single batch with tests checking playback of a short on-demand, from start (initial value) to end (valid value, then return to initial value). No need to check queue scenarios (with or without failed items in the queue).
  • Cleanup old implementation.
  • Fix unit tests for playlist navigation in presence of failure (must be able to go back over failed items).
  • Error: Must persist last error in the player (since we can hook any view at any time to an existing player, we must be able to reflect the current error state if needed). @Published property?
  • AVPlayer vs. AVQueuePlayer vs. QueuePlayer publishers. Is any separation in extensions meaningful?
  • Rename MediaSelectionContext as MediaSelectionProperties?
  • Time range return value consistency: Should we always return .invalid instead of empty ranges?
  • Create issue to think about the playback speed API, most notably in the context of stream tracker implementation (did publish trick should best be avoided).
  • Create issue to consolidate Control Center publishers (both pipelines use the current item publisher).
  • Create issue to remove AirPlay playlist tricks (AirPlay is broken in playlists on iOS 17 anyway).
  • Create issue to fix performance issue associated with media selection option sorting.
  • Create issue to possibly improve tracker API (subscription to publishers made in CurrentItem and only protocol methods called when changes are published?).
  • Report broken Control Center cleanup in iOS 17.1 to Apple.
  • Fix onReceive superfluous subscription creation.
  • Fix identified issues:
    • Seek bar jump in the Control Center when seeking while paused.

@defagos
Copy link
Member Author

defagos commented Sep 26, 2023

We have removed all the code we had to manage errors in playlists. Now back navigation will get stuck on failed items but this should be a corner case. In most cases the user will probably have another way of getting back to items prior to the failed one anyway (e.g. a playlist item selection interface).

For the moment we decided to keep things simple. We will improve the situation later if the need arises.

@defagos
Copy link
Member Author

defagos commented Sep 27, 2023

Regarding import optimization, some imports are superfluous when they involve a category (in this case any file in the same module can perform the import so that the code compiles). This is a compiler bug but imports can still be optimized in the following way:

  1. Check minimal imports required.
  2. Look for public category method use and add non-required import (syntax coloring will then be fixed).

In some cases, e.g. where we have to import Core and SRGDataProviderCombine in the demo, we could use this fact to our advantage when only category methods were accessed. This allowed us to fix ambiguities without module prefixing but is fragile at best.

We decided to perform only 1 and to currently use the compiler bug to our advantage (we won't also import Foundation anymore explicitly). Once it is fixed (if it is ever fixed) we will easily discover the missing imports at compilation time.

Strict imports are still not addressed but there are likely workarounds for cleanup.

@waliid waliid linked a pull request Oct 6, 2023 that will close this issue
5 tasks
@defagos defagos moved this from 🚧 In Progress to 🍿 Code Review in Pillarbox Oct 6, 2023
@github-project-automation github-project-automation bot moved this from 🍿 Code Review to ✅ Done in Pillarbox Oct 6, 2023
@defagos
Copy link
Member Author

defagos commented Oct 6, 2023

We have entirely revisited our reactive player implementation:

  • All playback-related state is now consolidated using a single publisher into a PlayerProperties.
  • Only some properties are seen as essential state (core properties) and lead to the Player publishing changes automatically. These properties can be directly read from a Player instance.
  • Other properties can be read from a publisher which uses multicast so that each player instance only has one state publisher shared among all subscribers. This ensures consistent state throughout subscribers while avoiding costly publisher multiplication with each subscription.
  • Local state extraction in SwiftUI views can be made with an onReceive(player:at:to:) modifier. This ensures that view hierarchies can be optimized so that frequent updates are only performed locally.

Some work remains to possibly consolidate more state (items and playback speed) but this has been deferred to other issues.

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
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants