[vlc-devel] [PATCH 2/2] input: don't override "time" and "position" when user seeks

Thomas Guillem thomas at gllm.fr
Fri Jun 10 16:03:44 CEST 2016



On Fri, Jun 10, 2016, at 15:36, Rémi Denis-Courmont wrote:
> 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.

That's the most annoying bug (for me) !

> 
> 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).

This issue is still easily reproducible with UHD videos on desktop. It
happens a lot on VLC ports. I tried to do a hack in the android port,
but it's not a perfect one (and it's ugly)...

> 
> > 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.

In my first implementation, I incremented/decremented the atomic for all
seeks requests (ControlIsSeekRequest()). But some seek requests are
pushed internally and are not updating "time" and "position". I didn't
really know what to do with that, maybe this is not an issue...

> 
> > ---
> >  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/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel


More information about the vlc-devel mailing list