SRGSSR / pillarbox-apple

A next-generation reactive media playback ecosystem for Apple platforms.
https://testflight.apple.com/join/TS6ngLqf
MIT License
45 stars 6 forks source link

Control Center metadata may disappear on current item deletion #785

Closed defagos closed 2 months ago

defagos commented 2 months ago

Description of the problem

When deleting the item currently being played from a playlist, the metadata displayed in the Control Center might vanish (time information stays correct, though).

Relevant stack trace or log output

No response

Reproducibility

Always

Steps to reproduce

  1. Open a URN playlist example.
  2. Tap on some item to play it (in the middle of the list).
  3. Tap on an item located before the item you just selected (this is why we started in the middle).
  4. Delete the current item.
  5. Check the Control Center. The metadata is missing.

This is really an issue in our code since the metadata attached to the now playing center is missing when logged to the console.

Library version

0.10.0

Operating system

iOS 17

Code sample

No response

Is there an existing issue for this?

defagos commented 2 months ago

This is a regression introduced in #769.

defagos commented 2 months ago

The issue is related to ordering of registrations in multicasting:

Here is the sample project I used. When run you can click on the playlist update button and observe that what is received before sharing to multiple subscribers does not match what is received afterwards:

--> [before] items = ["A", "B"], curr = nil
--> [before] items = ["A", "B"], curr = Optional("A")
--> [before] items = ["B"], curr = Optional("A")
--> [before] items = ["B"], curr = Optional("B")

vs.

--> [after] items = ["A", "B"], curr = nil
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")

By reverting the subscription order (configureQueueUpdatesFixed method available in the sample project) the result is the expected one:

--> [after] items = ["A", "B"], curr = nil
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["A", "B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("B")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")
--> [after] items = ["B"], curr = Optional("A")
defagos commented 2 months ago

There is a clear difference in behavior between the following cases:

The first is mostly related to current item updates (though lazy loading can later lead to asset updates), while the second is related to stored item updates (and thus to asset updates).

It also means we can simply reproduce the very same issue as follows:

  1. Open a playlist demo.
  2. Reorder the item after the one being played (i.e. already preloaded) with another random item after it.
  3. Delete the item being played.

Maybe careful analysis of what happens in this simpler case might provide useful insights into the actual problem. Note that reordering of two random items further down in the list does not lead to the issue.

defagos commented 2 months ago

I could properly debug and understand the origin of the issue. As initially suspected this is because of an update loop involving ReplaySubject.

Analysis

Let us assume we have the following a publisher which shares its values to multiple suscribers with a multicast. A ReplaySubject is inserted in the middle so that all subscribers can receive recent values upon subscription:

   ┌──────────────────────────┐    ┌────────────────────────┐    ┌─────────────────────┐   
┌─▶│        Publisher         │───▶│     ReplaySubject      │─┬─▶│    Subscriber 1     │   
│  └──────────────────────────┘    └────────────────────────┘ │  └─────────────────────┘   
└─────────────────────────────────────────────────────────────┼───────────────────────────┐
                             on same                          │  ┌─────────────────────┐  │
                             thread                           ├─▶│    Subscriber 2     │──┘
                                                              │  └─────────────────────┘   
                                                              │                            
                                                              │  ┌─────────────────────┐   
                                                    Multicast ├─▶│    Subscriber 3     │   
                                                              │  └─────────────────────┘   
                                                              │                            
                                                              │  ┌─────────────────────┐   
                                                              └─▶│    Subscriber 4     │   
                                                                 └─────────────────────┘   

In addition let us assume that the 2nd subscriber might trigger the source publisher again on the same thread. We also assume that this recursion has an exit condition, which means the 2nd subscriber will at some point stop calling the source publisher again.

The situation above models what we currently have in our Player implementation:

Here is what might happen with the above setup:

In other words here is how the subscribers receives values, in order:

1A, 2A, 1B, 2B, 3B, 4B, 3A, 4A

This means that the 3rd and 4th subscribers, located after the subscriber leading to a recursive call, receive old values after the newest one. This is exactly what we observe in our implementation and why we receive an old update, leading to the Control Center not finding an item to display.

In the sample code attached above, it is mentioned that reordering the publishers can solve this issue. By putting the publisher leading to the recursive call last, we namely can have the expected call order:

1A, 2A, 3A, 4A, 1B, 2B, 3B, 4B

This works but is of course extremely fragile.

Remark

I could not find any documentation stating what should happen in such cases. All ReplaySubject implementations I checked are affected in the same way (CombineExt, e.g.), including Combine share() operator (which has no values to replay).

Possible solutions

Here are a few possible solutions:

Reorder subscriptions

As written above we could reorder the subscriptions but this is fragile and, in our case, the Control Center related subscribers can be toggled at any time (isActive). Not practical.

Remove the subscriber / publisher recursion

We could remove the recursion, e.g. with a receive(on: DispatchQueue.main), but this introduces an entire new class of issues as well. This also would require a lot of APIs to work differently (e.g. the currentIndex would not be immediately correct anymore) and tests to be adjusted accordingly.

Improve ReplaySubject

Our implementation, like others, uses NSRecursiveLock to support recursive calls, but we can be more careful by processing pending values first. I am pretty sure we can write good unit tests to support this case and fix the implementation accordingly.

The fixed implementation could also be used as a safe alternative to share() when the size of the buffer is set to 0.

defagos commented 2 months ago

Here is how we can fix our current ReplaySubject to support recursive calls from subscribers (e.g. sinks):

  1. To avoid processing subscriptions out-of-order we must provide any new value to all subscriptions before actually sending it to subscribers. To achieve this result we loop over all subscriptions and just append the values to their internal DemandBuffer but, instead of sending what is returned by the demand buffer directly, we append the values temporarily to a second buffer pendingValues.
  2. After all subscriptions have been provided with the new value we send all pending values in a second pass. To avoid piling up the same values over if a subscriber triggers a recursion on the same thread as a result of the receive, we must first flush this 2nd buffer and keep the pending values locally.

This approach guarantees that each subscriber receives values in the order they are provided to the subject.

It does not guarantee any relative order between subscribers, though. For example if we have three subscribers like above, with the 2nd one possibly leading to a recursion on the same thread, the order we will get is 1A, 2A, 1B, 2B, 3A, 3B, not 1A, 2A, 3A, 1B, 2B, 3B. This is not an issue, though, since what is important is that each subscriber individually receives values in-order.