[vlc-devel] [PATCH 1/3] Improve low FPS support.

[Rémi Denis-Courmont]

Le 2014-11-25 10:30, Paulo Vitor Magacho da Silva a écrit :
> ---
>  modules/access/live555.cpp      | 13 +++++++------
>  modules/codec/avcodec/video.c   |  4 ++--
>  src/input/clock.c               |  2 +-
>  src/video_output/video_output.c |  2 +-
>  4 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/modules/access/live555.cpp b/modules/access/live555.cpp
> index b14898c..4442b9b 100644
> --- a/modules/access/live555.cpp
> +++ b/modules/access/live555.cpp
> @@ -1217,8 +1217,9 @@ static int Play( demux_t *p_demux )
>      {
>          /* The PLAY */
>          p_sys->rtsp->sendPlayCommand( *p_sys->ms,
> default_live555_callback, p_sys->f_npt_start, -1, 1 );
> +        const int i_timeout = var_InheritInteger( p_demux, 
> "ipv4-timeout" );
>
> -        if( !wait_Live555_response(p_demux) )
> +        if( !wait_Live555_response(p_demux, i_timeout) )

I don't see how this fits with ipv4-timeout.

>          {
>              msg_Err( p_demux, "RTSP PLAY failed %s",
> p_sys->env->getResultMsg() );
>              return VLC_EGENERIC;
> @@ -1386,14 +1387,14 @@ static int Demux( demux_t *p_demux )
>          */
>      }
>      else if( !p_sys->b_multicast && !p_sys->b_paused &&
> -              p_sys->b_no_data && ( p_sys->i_no_data_ti > 34 ) )
> +              p_sys->b_no_data && ( p_sys->i_no_data_ti > 68 ) )
>      {
>          bool b_rtsp_tcp = var_GetBool( p_demux, "rtsp-tcp" ) ||
>                                  var_GetBool( p_demux, "rtsp-http" );
>
>          if( !b_rtsp_tcp && p_sys->rtsp && p_sys->ms )
>          {
> -            msg_Warn( p_demux, "no data received in 10s. Switching
> to TCP" );
> +            msg_Warn( p_demux, "no data received in 20s. Switching
> to TCP" );

This is way too long. Even 10 seconds is already long.

And this has nothing to do with FPS and everything to do with network 
conditions.

>              if( RollOverTcp( p_demux ) )
>              {
>                  msg_Err( p_demux, "TCP rollover failed, aborting" );
> @@ -1401,14 +1402,14 @@ static int Demux( demux_t *p_demux )
>              }
>              return 1;
>          }
> -        msg_Err( p_demux, "no data received in 10s, aborting" );
> +        msg_Err( p_demux, "no data received in 20s, aborting" );
>          return 0;
>      }
>      else if( !p_sys->b_multicast && !p_sys->b_paused &&
> -             ( p_sys->i_no_data_ti > 34 ) )
> +             ( p_sys->i_no_data_ti > 68 ) )
>      {
>          /* EOF ? */
> -        msg_Warn( p_demux, "no data received in 10s, eof ?" );
> +        msg_Warn( p_demux, "no data received in 20s, eof ?" );
>          return 0;
>      }
>      return p_sys->b_error ? 0 : 1;
> diff --git a/modules/codec/avcodec/video.c 
> b/modules/codec/avcodec/video.c
> index 5ba6e2e..fafe4f9 100644
> --- a/modules/codec/avcodec/video.c
> +++ b/modules/codec/avcodec/video.c
> @@ -515,7 +515,7 @@ static picture_t *DecodeVideo( decoder_t *p_dec,
> block_t **pp_block )
>      }
>
>      if( !p_dec->b_pace_control && (p_sys->i_late_frames > 0) &&
> -        (mdate() - p_sys->i_late_frames_start > INT64_C(5000000)) )
> +        (mdate() - p_sys->i_late_frames_start > INT64_C(60000000)) )

No way. If you want to support arbitrarily low frame rate, you do it 
properly. Tweaking magic values like this is a recipe for huge problems.

>      {
>          if( p_sys->i_pts > VLC_TS_INVALID )
>          {
> @@ -524,7 +524,7 @@ static picture_t *DecodeVideo( decoder_t *p_dec,
> block_t **pp_block )
>          if( p_block )
>              block_Release( p_block );
>          p_sys->i_late_frames--;
> -        msg_Err( p_dec, "more than 5 seconds of late video -> "
> +        msg_Err( p_dec, "more than 60 seconds of late video -> "
>                   "dropping frame (computer too slow ?)" );
>          return NULL;
>      }
> diff --git a/src/input/clock.c b/src/input/clock.c
> index 1419f8f..8f4acf9 100644
> --- a/src/input/clock.c
> +++ b/src/input/clock.c
> @@ -430,7 +430,7 @@ int input_clock_ConvertTS( input_clock_t *cl,
>      /* */
>      if( *pi_ts0 > VLC_TS_INVALID )
>      {
> -        *pi_ts0 = ClockStreamToSystem( cl, *pi_ts0 + AvgGet( 
> &cl->drift ) );
> +        *pi_ts0 = ClockStreamToSystem( cl, *pi_ts0 /* AvgGet(
> &cl->drift ) */ );

This kind of changes is totally unacceptable without explanations why 
the current code is wrong and why the patch is right.

>          if( *pi_ts0 > cl->i_ts_max )
>              cl->i_ts_max = *pi_ts0;
>          *pi_ts0 += i_ts_delay;
> diff --git a/src/video_output/video_output.c
> b/src/video_output/video_output.c
> index 2a6fe2a..820199d 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -66,7 +66,7 @@ static void VoutDestructor(vlc_object_t *);
>  /**
>   * Late pictures having a delay higher than this value are thrashed.
>   */
> -#define VOUT_DISPLAY_LATE_THRESHOLD (INT64_C(20000))
> +#define VOUT_DISPLAY_LATE_THRESHOLD (INT64_C(5000000))

See above. 60ms is the absolute maximum acceptable value here, IMHO.

>
>  /* Better be in advance when awakening than late... */
>  #define VOUT_MWAIT_TOLERANCE (INT64_C(4000))

-- 
Rémi Denis-Courmont