Skip to content
Snippets Groups Projects

player: input: release assertions on player state

  1. May 12, 2022
    • Alexandre Janniaux's avatar
      player: input: release assertions on player state · 84659b15
      Alexandre Janniaux authored and Hugo Beauzée-Luyssen's avatar Hugo Beauzée-Luyssen committed
      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
      84659b15
Loading