<p>Hi Rémi,</p>
<p>On 16/08/06 21:12, Rémi Denis-Courmont wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> vlc | branch: master | Rémi Denis-Courmont <remi@remlab.net>
| Sat Aug 6 22:02:57 2016 +0300| [81bb5426da3bd49cd05c190dcaed1ece0983d43d]
| committer: Rémi Denis-Courmont
live555: use timer for time-out prevention</code></pre>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<h3 id="httpgit.videolan.orggitweb.cgivlc.gitacommith81bb5426da3bd49cd05c190dcaed1ece0983d43d">http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=81bb5426da3bd49cd05c190dcaed1ece0983d43d</h3>
</blockquote>
</blockquote>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="debug-diagnostic-vs-error">debug diagnostic vs error</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> @@ -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!" );</code></pre>
</blockquote>
<p>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 <code>msg_Err</code> to <code>msg_Dbg</code> with a more descriptive message.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="unnecessary-variable">Unnecessary variable</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> @@ -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 );</code></pre>
</blockquote>
<p>Given that <code>RTSPClient::sendGetParameterCommand</code> does not accept it’s third parameter by reference, simply passing <code>NULL</code> instead of <code>bye</code> is equivalent, and as such the usage of <code>bye</code> is redundant.</p>
<hr style="height:1px;margin-bottom:20px;background-color:#ddd;color:#ddd" />
<h3 id="waiting-for-reply-on-error">Waiting for reply on error</h3>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 */
}</code></pre>
</blockquote>
<p>If we do not respect the values from either of the two <code>rtsp->send*</code>, might this not lead to us waiting indefinitely for a reply that never happens due to the sending failing (for whatever reason)?</p>
<p>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.</p>