[vlc-devel] [PATCH v2 3/3] fingerprinter: use vlc_cond_t instead of sleep

[Alexandre Janniaux]

Hi,

I fixed the issues mentioned by Rémi and added information
about the previous state of the file.

I didn't send the thread cancellation -> boolean patch though
because it was not very simplifying anything (+16 -11) and it
needed an additional atomic as the lock is not shared between
the incoming queue steal at the beginning and the processing
queue later.

I'll send it in a separate patchset afterwards to have more
opinion on it, but I don't think mine is for removal of this
thread cancellation code.

Regards,
--
Alexandre Janniaux
Videolabs

On Mon, Mar 23, 2020 at 07:03:42PM +0100, Alexandre Janniaux wrote:
> Instead of checking new requests every seconds, wait for actual new
> requests to be enqueued before trying to do any processing. This avoid
> the harmful sleep warning on vlc_tick_sleep as well as providing better
> latency for new fingerprinting requests on a cold fingerprinter.
>
> The vlc_tick_sleep() was a vlc_cond_timedwait before and became a msleep
> since 832d5a6d7435397f178c3455e70995ea5752fad0, so there was no clear
> reason why it is there.
> ---
>  modules/misc/fingerprinter.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/modules/misc/fingerprinter.c b/modules/misc/fingerprinter.c
> index 99a05346f0..fa568d6cd7 100644
> --- a/modules/misc/fingerprinter.c
> +++ b/modules/misc/fingerprinter.c
> @@ -51,6 +51,8 @@ struct fingerprinter_sys_t
>          vlc_mutex_t         lock;
>      } incoming, results;
>
> +    vlc_cond_t              incoming_cond;
> +
>      struct
>      {
>          vlc_array_t         queue;
> @@ -85,14 +87,13 @@ static int EnqueueRequest( fingerprinter_thread_t *f, fingerprint_request_t *r )
>      fingerprinter_sys_t *p_sys = f->p_sys;
>      vlc_mutex_lock( &p_sys->incoming.lock );
>      int i_ret = vlc_array_append( &p_sys->incoming.queue, r );
> +    vlc_cond_signal( &p_sys->incoming_cond );
>      vlc_mutex_unlock( &p_sys->incoming.lock );
>      return i_ret;
>  }
>
>  static void QueueIncomingRequests( fingerprinter_sys_t *p_sys )
>  {
> -    vlc_mutex_lock( &p_sys->incoming.lock );
> -
>      for( size_t i = vlc_array_count( &p_sys->incoming.queue ); i > 0 ; i-- )
>      {
>          fingerprint_request_t *r = vlc_array_item_at_index( &p_sys->incoming.queue, i - 1 );
> @@ -100,7 +101,6 @@ static void QueueIncomingRequests( fingerprinter_sys_t *p_sys )
>              fingerprint_request_Delete( r );
>      }
>      vlc_array_clear( &p_sys->incoming.queue );
> -    vlc_mutex_unlock(&p_sys->incoming.lock);
>  }
>
>  static fingerprint_request_t * GetResult( fingerprinter_thread_t *f )
> @@ -250,6 +250,7 @@ static int Open(vlc_object_t *p_this)
>
>      vlc_array_init( &p_sys->incoming.queue );
>      vlc_mutex_init( &p_sys->incoming.lock );
> +    vlc_cond_init( &p_sys->incoming_cond );
>
>      vlc_array_init( &p_sys->processing.queue );
>      vlc_cond_init( &p_sys->processing.cond );
> @@ -344,11 +345,16 @@ static void *Run( void *opaque )
>      /* main loop */
>      for (;;)
>      {
> -        vlc_tick_sleep( VLC_TICK_FROM_SEC(1) );
> +        vlc_mutex_lock( &p_sys->incoming.lock );
> +        mutex_cleanup_push( &p_sys->incoming.lock );
> +
> +        while( vlc_array_count( &p_sys->incoming.queue ) == 0 )
> +            vlc_cond_wait( &p_sys->incoming_cond, &p_sys->incoming.lock );
>
>          QueueIncomingRequests( p_sys );
>
> -        vlc_testcancel();
> +        vlc_cleanup_pop();
> +        vlc_mutex_unlock( &p_sys->incoming.lock );
>
>          bool results_available = false;
>          while( vlc_array_count( &p_sys->processing.queue ) )
> --
> 2.25.2
>