libvlc/player: Add a new media stopping event
- Sep 26, 2023
-
-
The previous patches added an event from libvlc_media_player_t which notify the application that the current media is stopping, regardless of whether the player itself is stopping or that it will process the next media. Those patches solve a design problem on the imem-access and libvlc callbacks where the application could not be notified that the input was stopped when the read() callback was pending, which happens through vlc_interrupt_t in other demux. In that situation, an application wanting to trigger the stop of the media player can only signal the media end-of-stream, triggering a pipeline drain, before stopping the player. ```mermaid sequenceDiagram box Orange VLC side participant input as Input thread participant imem as imem access end box Cyan Application size participant app as Application end input ->> imem: stream::read() activate imem imem ->> app: media callback read() app --x imem: return "EOS" (no data to send) imem --x input: Trigger pipeline drain (EOS) deactivate imem app ->> input: stop (change media) ```mermaid This decoder and output drain adds latency to the change of a media. The duration depends on condition but I've been able to reproduce a steady 400ms latency addition when changing media (meaning that it will take 400ms more to start reading the new media), up to a gigantic 1.6s in a randomized manner (1.1s being taken by the drain of the audio decoder!). With the new system, it's now possible to let the application respond to the new event to handle the interruption and termination of what it has been doing in the libvlc_media_read_cb function, without triggering the end of the media when it would have triggered a drain. ```mermaid sequenceDiagram box Orange VLC side participant destructor as Player input destructor participant input as Input participant imem as imem access end box Cyan Application side participant player as Player participant app as Application end par Input thread activate imem input ->> imem: stream::read() imem ->> app: media callback read_cb() app ->> app: read_cb() might be waiting for some data and Application thread app ->> player: Stop() player ->> destructor: Add current input to destructor and Player destructor thread destructor ->> input: input_Stop() destructor ->> app: libvlc_MediaPlayerMediaStopping event end app ->> app: Interrupt read_cb() and return EOF deactivate imem ``` This commit verifies this behaviour in a reliable way to ensure that the problem is indeed solved by this approach, while allowing to document the solution on the application side. To do so, two different tests are made: - A sanity test which checks that we can stop a libvlc media which is not blocking, basically checking that the media_callback API works and running it when we want to check against sanitizers. - A specific test checking that we can block the read() and only unblock it when the libvlc_MediaPlayerMediaStopping event is emitted, checking that the documented behaviour for the media callbacks is working correctly. --- The test needs: - No intermediate cache so that we clearly control the access from the demux. - Better deterministic behaviour, typically to check that we don't call AccessReadNonBlocking twice if we don't notify between them and try to check that read() is correctly terminating in this case. --- On a side note, other solutions were investigated to solve this issue: 1/ Provide an `errno` variable as parameter to read: By providing an errno parameter, we can provide a specific error code when -1 is returned, and in particular use EAGAIN/EWOULDBLOCK to inform the imem-access code that we don't have data yet. Then, the imem-access can wait for a notification from the application to start reading again, while listening to interruption coming from the core. This solution was submitted but it felt like alienating the libVLC imem API and libvlc media API to solve this problem, without considering it as an application problem as a whole. 2/ Provide the same solution but use `errno` directly: This solution would be the simplest but there's no way for the application to realize that errno returning EAGAIN, ie. mapping the read callback directly to read() on a non-blocking socket for instance, might create silent regression where the access would not try to fetch more data, since the application won't know it must notify the access. 3/ Provide a poll_cb() callback in the imem interface The solution would require another callback, instead of another parameter. The new callback would be called every time before the read() callback, and would notify whether the read() is ready or not. This adds a new way for the application to exit, differentiating the return code for read() from the return code from poll(), but the access still cannot be interrupted and the additional cost on function call is higher. 4/ Provide a `waker` assignment (like rust futures) Provide a new callback to specify an object that can be used to notify the access that new data is available. This is much like poll() but it has the benefit that the application can immediately notify that no data is available, and then provide the object to notify the arrival of data asynchronously without exposing a public function on libvlc_media_t. This is still quite complex and still has a heavy function call cost, but it also fullfill every points from the suggestion here. However, the rust async model is not really the idiomatic model in C compared to POSIX errno. 5/ Solve the problem at the decoder and output level One of the main symptoms that can be observed when changing media is the increase of the time to close a media, and in particular, to drain the decoder. This is also a non-interruptible task, because a decoder drain is synchronous. Admittedly, the interruption problem is less problematic if the drain itself can be handled smoothly afterwards, and I actually think both problems need to be solved eventually. But the decoder behaviour on drain depends on the decoder implementation, whereas the imem-access suggests a solution on the application side to correctly express the required intent, and it is also quite suitable to express an injection workflow (data being written from the application to the VLC pipeline) as well as a pull workflow (data being read from the application by VLC).
30d1237d -
Signal that the media is stopping to the libvlc client. This is done after notifying the media on the VLC side that we're going to stop it, so that application can rely on the fact that the objects using application resources are already stopping, so that we can trigger actions like EOS, closing those resources, etc.
3841df7a -
Signal that the media is stopping to the client. This is done after input_Stop so that application can rely on the fact that the underlying input_thread is already stopping, so that we can trigger actions like EOS, closing input resources, etc.
e651ac80 -
Splitting the function allows return statement, which removes the need for break, and simplify the code. It also allows to use a designated initializer for the event structure. The code also move the vlc_assert_unreachable in the new function. By moving the vlc_assert_unreachable after the switch(), we can get warnings when not every cases are handled since there is no default case, and we can keep asserting when it fails to reach any return statement before.
6d40b91c -
By moving the vlc_assert_unreachable after the switch(), we can get warnings when not every cases are handled since there is no default case, and we can keep asserting when it fails to reach any return statement before.
4a35193e -
The function improves the error reporting when the assertion is triggered, and also logs what were the resulting state, while nicely handling negative index like python arrays.
52a231b8 -
Dumping the list of state helps finding the root cause of the issue when a test fails, but also enable additional manual tests to check whether an unexpected untested test transition has happened or not.
bcba4461 -
Most of the code is checking the player state, but we had no way to signal in the test logs which state was actually there when it fails.
9bb751cb -
The event forwards the vlc_player_t event `on_stopping_current_media` which signals that the media actually playing inside the player is stopping. The event is essential to the `libvlc_media_new_callbacks` API since there's no way to stop the \ref libvlc_media_read_cb callback without triggering an EOF, and the EOF should be signalled only as soon as the media is stopping.
4ac61be1 -
Add a new event notifying when the current media is stopping. The media can be stopping without the player being stopping, ie. when the media is finished and the player prepares the next one, but also when the player changes media while having the player in the VLC_PLAYER_STATE_PLAYING state. The event will also be fired when the player is simply stopping, in which case we expect that VLC_PLAYER_STATE_STOPPING is signaled before the input is actually stop, which is coherent with the current behaviour.
2ae6636c -
Designated initializer will allow to add new fields in the vlc_player_cbs structure while automatically initializing the new fields to null.
5d502b0b -
d286d255
-
The lambda will allow to use a zero-initializer and only initialize the required fields.
df4ec732
-