Closed defagos closed 1 year ago
Here are the item properties that currently require the item to be ready to play before being meaningful:
In fact these are all properties of interest for an item, it is therefore likely we should consolidate everything item-related.
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:
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.Properly implementing this architecture is quite some work, though. We should:
$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.AVQueuePlayer
observation, and the queue player itself is kept synchronized with the stored items in a unidirectional way.$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:
Things to do:
Properties
instead).isBuffering
)public
.@Published
property?AVPlayer
vs. AVQueuePlayer
vs. QueuePlayer
publishers. Is any separation in extensions meaningful?MediaSelectionContext
as MediaSelectionProperties
?.invalid
instead of empty ranges?CurrentItem
and only protocol methods called when changes are published?).onReceive
superfluous subscription creation.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.
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:
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.
We have entirely revisited our reactive player implementation:
PlayerProperties
.Player
publishing changes automatically. These properties can be directly read from a Player
instance.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.
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:
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
Tasks