[vlc-devel] [PATCH 2/2] input: don't override "time" and "position" when user seeks
Rémi Denis-Courmont
remi at remlab.net
Fri Jun 10 15:36:54 CEST 2016
Le 2016-06-10 15:27, Thomas Guillem a écrit :
> The "time" and the "position" vars of the input_thread are mainly
> used to seek from a user request. When these vars change, this
> trigger a callback that push a seek control to the input_thread_t.
Yes, that's a bug. time and position variables mean two different
things at the same time:
- what time the user wants to seek to, and
- what is the current playback time.
Obivously, that can't work.
To avoid triggering its own callbacks, the input thread uses a nasty
trick: it calls var_Change(VLC_VAR_SETVALUE) instead of var_Set().
> The input_thread_t could override theses var before the seek controls
> were
> processed (from MainLoopStatistics()). This could lead to an UI
> glitch: the
> seek bar goes back to the previous position and then return to the
> seek
> position when the control is processed.
There are no ways to avoid that. Someone (Derk Jan, IIRC) already added
a work around for this glitch aeons ago, which mostly works. But there
is always a race where a the value of the variable(s) is queried right
after a seek request, before the input thread has had the time to adjust
the value(s).
> To fix this issue, I added 2 new seek controls that are only pushed
> from the
> "time" and "position" callbacks. When those events are pushed, "time"
> and
> "position" can't be updated until user seek controls are processed.
I don't see why you need two new controls. Controls should only be
emitted when the "user" requested a seek. Having two sets of controls
doing the same thing is bizarre.
> ---
> src/input/event.c | 4 ++++
> src/input/input.c | 27 +++++++++++++++++++++++++++
> src/input/input_internal.h | 3 +++
> src/input/var.c | 4 ++--
> 4 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/src/input/event.c b/src/input/event.c
> index 456c4d7..e62409f 100644
> --- a/src/input/event.c
> +++ b/src/input/event.c
> @@ -60,6 +60,10 @@ void input_SendEventPosition( input_thread_t
> *p_input, double f_position, mtime_
> {
> vlc_value_t val;
>
> + /* Don't override time and position when user asked to seek */
> + if( atomic_load( &p_input->p->user_seek_count ) != 0 )
> + return;
> +
> /* */
> val.f_float = f_position;
> var_Change( p_input, "position", VLC_VAR_SETVALUE, &val, NULL );
> diff --git a/src/input/input.c b/src/input/input.c
> index fe49b01..5189f28 100644
> --- a/src/input/input.c
> +++ b/src/input/input.c
> @@ -67,6 +67,7 @@ static void MainLoop( input_thread_t
> *p_input, bool b_interactive );
> static inline int ControlPop( input_thread_t *, int *, vlc_value_t
> *, mtime_t i_deadline, bool b_postpone_seek );
> static void ControlRelease( int i_type, vlc_value_t val );
> static bool ControlIsSeekRequest( int i_type );
> +static bool ControlIsUserSeek( int i_type );
> static bool Control( input_thread_t *, int, vlc_value_t );
> static void ControlPause( input_thread_t *, mtime_t );
>
> @@ -319,6 +320,7 @@ static input_thread_t *Create( vlc_object_t
> *p_parent, input_item_t *p_item,
> p_input->p->is_stopped = false;
> p_input->p->b_recording = false;
> p_input->p->i_rate = INPUT_RATE_DEFAULT;
> + atomic_init( &p_input->p->user_seek_count, 0 );
> memset( &p_input->p->bookmark, 0, sizeof(p_input->p->bookmark)
> );
> TAB_INIT( p_input->p->i_bookmark, p_input->p->pp_bookmark );
> TAB_INIT( p_input->p->i_attachment, p_input->p->attachment );
> @@ -1533,6 +1535,8 @@ void input_ControlPush( input_thread_t
> *p_input,
> sys->control[sys->i_control++] = c;
>
> vlc_cond_signal( &sys->wait_control );
> + if( ControlIsUserSeek( i_type ) )
> + atomic_fetch_add( &sys->user_seek_count, 1 );
> }
> vlc_mutex_unlock( &sys->lock_control );
> }
> @@ -1549,7 +1553,9 @@ static int ControlGetReducedIndexLocked(
> input_thread_t *p_input )
> ( i_ct == INPUT_CONTROL_SET_STATE ||
> i_ct == INPUT_CONTROL_SET_RATE ||
> i_ct == INPUT_CONTROL_SET_POSITION ||
> + i_ct == INPUT_CONTROL_SET_USER_POSITION ||
> i_ct == INPUT_CONTROL_SET_TIME ||
> + i_ct == INPUT_CONTROL_SET_USER_TIME ||
> i_ct == INPUT_CONTROL_SET_PROGRAM ||
> i_ct == INPUT_CONTROL_SET_TITLE ||
> i_ct == INPUT_CONTROL_SET_SEEKPOINT ||
> @@ -1608,7 +1614,12 @@ static inline int ControlPop( input_thread_t
> *p_input,
> {
> /* Release Reduced controls */
> ControlRelease( p_sys->control[i].i_type,
> p_sys->control[i].val );
> +
> + if( ControlIsUserSeek( p_sys->control[i].i_type ) )
> + atomic_fetch_sub( &p_sys->user_seek_count, 1 );
> }
> + if( ControlIsUserSeek( p_sys->control[i_index].i_type ) )
> + atomic_fetch_sub( &p_sys->user_seek_count, 1 );
>
> /* */
> *pi_type = p_sys->control[i_index].i_type;
> @@ -1627,7 +1638,9 @@ static bool ControlIsSeekRequest( int i_type )
> switch( i_type )
> {
> case INPUT_CONTROL_SET_POSITION:
> + case INPUT_CONTROL_SET_USER_POSITION:
> case INPUT_CONTROL_SET_TIME:
> + case INPUT_CONTROL_SET_USER_TIME:
> case INPUT_CONTROL_SET_TITLE:
> case INPUT_CONTROL_SET_TITLE_NEXT:
> case INPUT_CONTROL_SET_TITLE_PREV:
> @@ -1648,6 +1661,18 @@ static bool ControlIsSeekRequest( int i_type )
> }
> }
>
> +static bool ControlIsUserSeek( int i_type )
> +{
> + switch( i_type )
> + {
> + case INPUT_CONTROL_SET_USER_POSITION:
> + case INPUT_CONTROL_SET_USER_TIME:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> static void ControlRelease( int i_type, vlc_value_t val )
> {
> switch( i_type )
> @@ -1725,6 +1750,7 @@ static bool Control( input_thread_t *p_input,
> switch( i_type )
> {
> case INPUT_CONTROL_SET_POSITION:
> + case INPUT_CONTROL_SET_USER_POSITION:
> {
> if( p_input->p->b_recording )
> {
> @@ -1757,6 +1783,7 @@ static bool Control( input_thread_t *p_input,
> }
>
> case INPUT_CONTROL_SET_TIME:
> + case INPUT_CONTROL_SET_USER_TIME:
> {
> int64_t i_time;
> int i_ret;
> diff --git a/src/input/input_internal.h b/src/input/input_internal.h
> index 9ee10e9..41a72fc 100644
> --- a/src/input/input_internal.h
> +++ b/src/input/input_internal.h
> @@ -92,6 +92,7 @@ struct input_thread_private_t
> bool is_stopped;
> bool b_recording;
> int i_rate;
> + atomic_uint user_seek_count;
>
> /* Playtime configuration and state */
> int64_t i_start; /* :start-time,0 by default */
> @@ -179,8 +180,10 @@ enum input_control_e
> INPUT_CONTROL_SET_RATE,
>
> INPUT_CONTROL_SET_POSITION,
> + INPUT_CONTROL_SET_USER_POSITION,
>
> INPUT_CONTROL_SET_TIME,
> + INPUT_CONTROL_SET_USER_TIME,
>
> INPUT_CONTROL_SET_PROGRAM,
>
> diff --git a/src/input/var.c b/src/input/var.c
> index 8c9dfb1..a29b9be 100644
> --- a/src/input/var.c
> +++ b/src/input/var.c
> @@ -609,7 +609,7 @@ static int PositionCallback( vlc_object_t
> *p_this, char const *psz_cmd,
> }
>
> /* */
> - input_ControlPush( p_input, INPUT_CONTROL_SET_POSITION,
> &newval );
> + input_ControlPush( p_input, INPUT_CONTROL_SET_USER_POSITION,
> &newval );
> }
> return VLC_SUCCESS;
> }
> @@ -635,7 +635,7 @@ static int TimeCallback( vlc_object_t *p_this,
> char const *psz_cmd,
> var_SetInteger( p_input, "intf-event", INPUT_EVENT_POSITION
> );
> }
>
> - input_ControlPush( p_input, INPUT_CONTROL_SET_TIME, &newval );
> + input_ControlPush( p_input, INPUT_CONTROL_SET_USER_TIME, &newval
> );
> return VLC_SUCCESS;
> }
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list