[vlc-commits] [Git][videolan/vlc][master] player: input: release assertions on player state
Hugo Beauzée-Luyssen (@chouquette)
gitlab at videolan.org
Thu May 12 17:17:26 UTC 2022
Hugo Beauzée-Luyssen pushed to branch master at VideoLAN / VLC
Commits:
84659b15 by Alexandre Janniaux at 2022-05-12T17:02:16+00:00
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
- - - - -
1 changed file:
- src/player/input.c
Changes:
=====================================
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;
View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/84659b1591bf26684f1047227f23e757f8cb4e3c
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/84659b1591bf26684f1047227f23e757f8cb4e3c
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list