player: input: release assertions on player state

The player code in src/player/player.c is setting player->started to true when starting a media with vlc_player_Start() and setting it to false when calling vlc_player_Stop().

However, vlc_player_Stop() will only queue the input for stopping, so the input is stopped asynchronously with no control from the player after this.

In the situation where we enabled :play-and-pause on the input, for instance using vlc_player_SetMediaStoppedAction(), the media will transition itself from PLAYING_S to PAUSE_S from the input thread mainloop, asynchronously from any vlc_player_Stop() call.

If vlc_player_Stop() is called soon enough for player->started to be set to false, but late enough so that the input thread still has the time to reach EOF and trigger pause, PAUSE_S will be reported to the player, which will handle it as VLC_PLAYER_STATE_PAUSED in the function vlc_player_input_HandleState, and it will expected the player and the input to be started, which is in this situation, not true.

Figure 1: interleaving leading to the assertion
|
|   PLAYER              DESTRUCTOR               CONTROL/INPUT
|
|         vlc_player_Stop()      input_Stop()          |
|      | ------------------> | ----------------> is_stopped = true
|      |                                               |
| player->started = false                          MainLoop
|      |                       ChangeState(PAUSE_S)    |
|      | <---------------------------------------------|
| assert(player->started)                              |
|                                           while (!input_Stopped(input))
|                                                      |
|                                               Input was supposed
|                                               to be stopped at this
|                                               point.

CONTROL/INPUT here represents both the control state, modified by input_Stop() and read at each MainLoop loop, and the input_thread running the MainLoop function.

Other solutions have been taken into account to solve the issues:

1/ Provide a separate set of boolean to track the playback state of the player separately from what the user requested (Start/Stop).

This is quite overlapping the existing player->global_state variable and it has been confusing to implement and read again. It does restrict the testing surface of the assertion anyway so it doesn't bring much compared to the submitted approach.

2/ Ensure in vlc_player_Stop() that input_Stop() has been called and check whether the input has been stopped before signalling PAUSE_S.

By far, it would have been my preferred method to prevent signaling the PAUSE_S state only in the case when it has already been stopped, meaning that the current assertion could have stayed the same, ie. that we could keep the testing surface on the player state too, but it's unfortunately not compatible with the current model.

input_Stop require the lock_control in order to modify the state of the input asynchronously, and we'd need input_Stop to wait while we would be checking the input state and sending the PAUSE_S state change event. In addition vlc_player_Stop and the player callback for input state change events are run under player lock.

So vlc_player_Stop() would lock the player (from the outside) and then the lock_control, whereas the input thread would lock the lock_control to check the state and then the player lock to report the event, leading to a lock inversion and thus deadlocks.

Figure 2: fixed interleaving
|
|   PLAYER                                       CONTROL/INPUT
|
|      |                                           MainLoop
|      |                                               |
|      |                                       [lock lock_control]
|      |                                        from input thread
|      |                                              ...
|      |
|      |  vlc_player_Stop()    input_Stop()
|      | ----------------------------------> [Waiting lock_control]
|      |
|      |                                              ...
|      |                       ChangeState(PAUSE_S)    |
|      | <---------------------------------------------|
| assert(player->started)                              |
|      |                                       [unlock lock_control]
|      |                                         from input thread
|      |                                              ...
|      |
|      |                                              ...
|      |                                               |
|      |                                      [locked lock_control]
|      |                                       from player thread
|      |                                               |
|      | -------------------------------------> is_stopped = true
| player->started = false                              |
|                                                      |
|                                           while (!input_Stopped(input))
|                                                      |
|                                              Input is now dead

3/ Reduce the scope of the assertion.

The current submission reduce the guarantees on the player state, which where checking that we couldn't call vlc_player_Pause from the player with a stopped player, and only check that the state reported by the input is still correct. It does check that we didn't reach END_S, or VLC_PLAYER_STATE_STOPPING in the player, when pausing though.

This is enough to fix the assertion, and not confusing to read in the code. Note that the check on input->started is also removed since input_Stop() will also stop the input asynchronously, leaving the possibility for the input to unroll to EOF regardless of whether input_Stop is called from vlc_player_Stop() or the destructor thread.

A test has been written to replicate this bug quite reliably on my machine, but because of the racy nature of this interleaving and the lack of infrastructure to test such interleaving directly in tests, it has been removed from this patch.

Fixes #26876 (closed)

src/player/input.c

@@ -234,7 +234,7 @@ vlc_player_input_HandleState(struct vlc_player_input *input,
             break;
 
         case VLC_PLAYER_STATE_PAUSED:
-            assert(player->started && input->started);
+            assert(player->global_state == VLC_PLAYER_STATE_PLAYING);
             assert(state_date != VLC_TICK_INVALID);
             input->pause_date = state_date;