[vlc-commits] [Git][videolan/vlc][master] player: handle ERROR_S from OPENING_S

[Jean-Baptiste Kempf (@jbk)]

Jean-Baptiste Kempf pushed to branch master at VideoLAN / VLC


Commits:
5b3b0f03 by Alexandre Janniaux at 2021-12-06T10:50:53+00:00
player: handle ERROR_S from OPENING_S

As of this patch, the state machine from the input is the following:

                     |INIT_S|

via input.c:Init()     ⬇   via input.c:Init()
 in case of error
        ⬇      ⬅   |OPENING_S|

        ⬇               ⬇  via input.c:Init()

     |ERROR_S|  ⬅  |PLAYING_S|  ⬅ ⮕  |PAUSE_S| --- ⮕  |ERROR_S|
   via Demux() error             via ControlPause/Unpause
        ⬇               ⬇
    if Init()  ⮕  via input.c:End()
    succeeded        |END_S|

Legend:
 - |STATE_S|: a state from the input_thread_t (see input_ChangeState)
 - via foobar: the internal function triggering the state transition

The transition from OPENING_S to ERROR_S was not handled correctly by
the vlc_player code.

After validation of hypothesis from Thomas, when a media reaches ERROR_S
from OPENING_S, because Init() from the file src/input/input.c failed,
END_S was never reached.

When vlc_player_SetCurrentMedia or vlc_player_Stop is called, the input
thread was added to the list of inputs to be stopped if the item was
started, or to the list of inputs to be joined if it wasn't started.

If the input wasn't started at all, no problem would arise, since we
don't need to wait for the END_S state.

However, when the input was started, and Init() failed, ie. we follow
the transition from OPENING_S to ERROR_S, the END_S state will never be
reached, and more precisely INPUT_EVENT_DEAD will be emitted without
END_S state being reached.

In the src/player/input.c code, INPUT_EVENT_DEAD means that the input
thread has reached the end of the thread function and is now joinable
without transitively waiting on the input thread waiting for a state
change. When this event is emitted, the input thread is added to the
list of input to be joined.

The assertion failing here signals that the input thread is scheduled
for joining although it has not been stopped yet.

   Assertion failed: (!vlc_list_HasInput(&player->destructor.inputs, input)),
   function vlc_player_destructor_AddInput, file player.c, line 165.

Three possible ways of solving the issue has been explored for the fix:

1/ Handle the transition in the destructor, allowing inputs to go from
the list of inputs to be stopped directly to the list of inputs to be
joined. This is by far the simplest method since it mostly consists of
removing from the list of inputs if it exists:

```
diff --git a/src/player/player.c b/src/player/player.c
+++ b/src/player/player.c
@@ -184,6 +184,9 @@ vlc_player_destructor_AddJoinableInput(vlc_player_t *player,
     if (vlc_list_HasInput(&player->destructor.stopping_inputs, input))
         vlc_list_remove(&input->node);

+    if (vlc_list_HasInput(&player->destructor.inputs, input))
+        vlc_list_remove(&input->node);
```

2/ Handle the transition in the input code, by explicitely moving from
the ERROR_S state to the END_S state to match the existing semantics
everywhere.

```
diff --git a/src/input/input.c b/src/input/input.c
+++ b/src/input/input.c
@@ -482,6 +482,10 @@ static void *Run( void *data )
         /* Clean up */
         End( p_input );
     }
+    else
+    {
+        input_ChangeState( p_input, END_S, VLC_TICK_INVALID );
+    }
```

3/ Handle the transition in the player code, by explicitely handling the
state transition from OPENING_S to ERROR_S. Since there are multiple
paths leading to ERROR_S, the PLAYING_S state is used as an indication
on whether this is a failure from OPENING_S and it can be considered as
STOPPED already. This is the version implemented in the current path.

The 3/ solution has been chosen mainly because:

 - In 1/, the state change in the destructor can be viewed as a
   defensive approach, and might allow previously invalid cases to
   happen while leading to multiple destruction in those cases.

 - In 2/, the state change from the input_thread would make sense to
   unify everywhere, but since there are different runloops for
   thumbnailer, preparser and playback, it would either lead to
   inconsistency or a lot of behaviour to check. In addition, though the
   input_thread exposes an ERROR_S state, the player handles the errors
   aside from the state and the input thread might change regarding
   that.

In both approaches, the vlc_player code cannot provide different next
media behaviour depending on whether there is an error from the opening,
not being able to differenciate between an invalid media and a media
running into an error during the playback.

The first approach has no context leading to that, and the second
approach would need more state tracking for that, which is exactly what
this patch provides without the changes in the input.

Finally, since the vlc_player is the most modern interface, addressing
the error state matching with the current design of the vlc_player is
what made the most sense to us.

Fixes #26344

Co-authored-by: Thomas Guillem <thomas at gllm.fr>

- - - - -


2 changed files:

- src/player/input.c
- src/player/player.h


Changes:

=====================================
src/player/input.c
=====================================
@@ -246,6 +246,7 @@ vlc_player_input_HandleStateEvent(struct vlc_player_input *input,
                                          VLC_TICK_INVALID);
             break;
         case PLAYING_S:
+            input->playing = true;
             vlc_player_input_HandleState(input, VLC_PLAYER_STATE_PLAYING,
                                          state_date);
             break;
@@ -254,6 +255,7 @@ vlc_player_input_HandleStateEvent(struct vlc_player_input *input,
                                          state_date);
             break;
         case END_S:
+            input->playing = false;
             vlc_player_input_HandleState(input, VLC_PLAYER_STATE_STOPPING,
                                          VLC_TICK_INVALID);
             vlc_player_destructor_AddStoppingInput(input->player, input);
@@ -266,6 +268,15 @@ vlc_player_input_HandleStateEvent(struct vlc_player_input *input,
                 input->error = VLC_PLAYER_ERROR_GENERIC;
                 vlc_player_SendEvent(input->player, on_error_changed, input->error);
             }
+            /* input->playing monitor the OPENING_S -> PLAYING_S transition.
+             * If input->playing is false, we have an error at the opening of
+             * the input thread and we won't reach END_S. */
+            if (!input->playing)
+            {
+                vlc_player_input_HandleState(input, VLC_PLAYER_STATE_STOPPING,
+                                             VLC_TICK_INVALID);
+                vlc_player_destructor_AddStoppingInput(input->player, input);
+            }
             break;
         default:
             vlc_assert_unreachable();
@@ -914,6 +925,7 @@ vlc_player_input_New(vlc_player_t *player, input_item_t *item)
 
     input->player = player;
     input->started = false;
+    input->playing = false;
 
     input->state = VLC_PLAYER_STATE_STOPPED;
     input->error = VLC_PLAYER_ERROR_NONE;


=====================================
src/player/player.h
=====================================
@@ -59,6 +59,9 @@ struct vlc_player_input
     vlc_player_t *player;
     bool started;
 
+    /* Monitor the OPENING_S -> PLAYING_S transition. */
+    bool playing;
+
     enum vlc_player_state state;
     enum vlc_player_error error;
     float rate;



View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/5b3b0f03b2744d8e9a626e9fb77f14a0723c83a0

-- 
View it on GitLab: https://code.videolan.org/videolan/vlc/-/commit/5b3b0f03b2744d8e9a626e9fb77f14a0723c83a0
You're receiving this email because of your account on code.videolan.org.