[vlc-devel] [PATCH] HLS: remove some useless mutex locks

[Hugo Beauzée-Luyssen]

2012/2/7 Frédéric Yhuel <fyhuel at viotech.net>:
> ---
>  modules/stream_filter/httplive.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/modules/stream_filter/httplive.c b/modules/stream_filter/httplive.c
> index c03061b..9afd5c0 100644
> --- a/modules/stream_filter/httplive.c
> +++ b/modules/stream_filter/httplive.c
> @@ -2070,9 +2070,7 @@ static void Close(vlc_object_t *p_this)
>     assert(p_sys->hls_stream);
>
>     /* */
> -    vlc_mutex_lock(&p_sys->download.lock_wait);
>     vlc_cond_signal(&p_sys->download.wait);
> -    vlc_mutex_unlock(&p_sys->download.lock_wait);
>

This mutex isn't useless, if you don't hold it before calling
vlc_cond_signal there is no way of being sure the thread is actually
waiting.
If the mutex is held by the download thread, it's likely to enter the
wait condition, thus the call to vlc_thread_join would stall.
Moreover, I think there are some slight chances of deadlock when
calling Close(), and some non thread-safe access to variables such as
as p_sys.(download|playback).segment...
Anyway, I'm pretty sure this mutex is not useless.

>     /* */
>     if (p_sys->b_live)
> @@ -2223,9 +2221,7 @@ static ssize_t hls_Read(stream_t *s, uint8_t *p_read, unsigned int i_read)
>             vlc_mutex_unlock(&segment->lock);
>
>             /* signal download thread */
> -            vlc_mutex_lock(&p_sys->download.lock_wait);
>             vlc_cond_signal(&p_sys->download.wait);
> -            vlc_mutex_unlock(&p_sys->download.lock_wait);

The same applies here, if the hls_Thread is about to enter
vlc_cond_wait (ie. having the download.lock held) if you don't acquire
the mutex first in the main thread, you might signal the thread before
it enters the vlc_cond_wait, thus leading to unexpected behavior.

>             continue;
>         }
>
> --
> 1.7.5.4
>

Please note that I am no threading expert, and therefore I'd love to
have a second opinion.

Best regards,

-- 
Hugo Beauzée-Luyssen