[vlc-devel] [vlc-commits] live555: use timer for time-out prevention

[Filip Roséen]

Hi Rémi,

On 16/08/06 21:12, Rémi Denis-Courmont wrote:

> vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net>
>     | Sat Aug  6 22:02:57 2016 +0300| [81bb5426da3bd49cd05c190dcaed1ece0983d43d]
>     | committer: Rémi Denis-Courmont
> 
> live555: use timer for time-out prevention
> 
> > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=81bb5426da3bd49cd05c190dcaed1ece0983d43d
> ---

--------------------------------------------------------------------------------

debug diagnostic vs error
-------------------------

> @@ -2118,41 +2065,47 @@ static void TaskInterruptData( void *p_private )
>  /*****************************************************************************
>   *
>   *****************************************************************************/
>
> [...]
>
> -    for( ;; )
> -    {
> -        /* Voodoo (= no) thread safety here! *Ahem* */
> -        if( p_timeout->b_handle_keep_alive )
> -        {
> -            /* Protect Live555 from us calling their functions simultaneously
> -               with Demux() or Control() */
> -            vlc_mutex_locker locker(&p_sys->timeout_mutex);
> -
> -            char *psz_bye = NULL;
> -            int canc = vlc_savecancel ();
> -
> -            p_sys->rtsp->sendGetParameterCommand( *p_sys->ms, default_live555_callback, psz_bye );
> +    msg_Err( p_demux, "fired!" );

I reckon the diagnostic above is a left-over from some debugging session,
and as it is not very helpful in terms of log-contents (nor an actual
error) I vote it to be changed from `msg_Err` to `msg_Dbg` with a more
descriptive message.

--------------------------------------------------------------------------------

Unnecessary variable
--------------------

> @@ -2118,41 +2065,47 @@ static void TaskInterruptData( void *p_private )
>  /*****************************************************************************
>   *
>   *****************************************************************************/
> -VLC_NORETURN
> -static void* TimeoutPrevention( void *p_data )
> +static void TimeoutPrevention( void *p_data )
>  {
> -    timeout_thread_t *p_timeout = (timeout_thread_t *)p_data;
> -    demux_t *p_demux = p_timeout->p_demux;
> +    demux_t *p_demux = (demux_t *) p_data;
>      demux_sys_t *p_sys = p_demux->p_sys;
> +    char *bye = NULL;
>
> [...]
>
> +    if( use_get_param )
> +        p_sys->rtsp->sendGetParameterCommand( *p_sys->ms,
> +                                              default_live555_callback, bye );

Given that `RTSPClient::sendGetParameterCommand` does not accept it's third
parameter by reference, simply passing `NULL` instead of `bye` is
equivalent, and as such the usage of `bye` is redundant.

--------------------------------------------------------------------------------

Waiting for reply on error
--------------------------

> +    if( use_get_param )
> +        p_sys->rtsp->sendGetParameterCommand( *p_sys->ms,
> +                                              default_live555_callback, bye );
> +    else
> +        p_sys->rtsp->sendOptionsCommand( default_live555_callback, NULL );
>  
> -        msleep (((int64_t)p_sys->i_timeout - 2) * CLOCK_FREQ);
> +    if( !wait_Live555_response( p_demux ) )
> +    {
> +        msg_Err( p_demux, "keep-alive failed: %s",
> +                 p_sys->env->getResultMsg() );
> +        /* Just continue, worst case is we get timed out later */
>      }

If we do not respect the values from either of the two `rtsp->send*`, might
this not lead to us waiting indefinitely for a reply that never happens due
to the sending failing (for whatever reason)?

This was, afaik, also present in the legacy implementation (and if the
problem exists within TimeoutPrevention it exists elsewhere too); but it
caught my eye when reading your patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20160806/fc7dd4d4/attachment.html>