[FFmpeg-devel] [PATCH] lavf: accept time base from untrusted codecs if it matches timings

[Anssi Hannula]

On 06.02.2011 08:32, Anssi Hannula wrote:
> If the first 4 frames contain timestamps that closely match the codec
> time base, accept the codec time base in tb_unreliable() even if the
> codec is blacklisted (currently MPEG-2 and H.264).
> 
> ---
> 
> Anssi Hannula wrote:
>> Here's a new patch that checks the timestamps of the first 4 frames
>> (using the same method which is used in the guess-framerate code) and
>> uses codec time base for r_frame_rate if the timestamps fit to it.
>>
>> I tested also with several other files, including H.264 PAFF, MVE
>> (ipmovie.c), and spotted no regressions.
>>
>> What do you think?
> 
> Ping. Also, I noticed an extra added newline in the patch, now removed.

This patch is indeed not enough. The h264 decoder may not discover the
time base immediately from the first few frames, so st->codec->time_base
may still be stream time base. So the code will notice from the first 4
frames that the stream time base can accurately represent timestamps
(which is always true), and sets codec_tb_matches_dts = 1.
Since the mpegts timebase is 1/90000 (i.e. too fine to be fps),
tb_unreliable() says (correctly) false despite codec_tb_matches_dts==1,
so the detection code continues to inspect frames.

When the h264 decoder finally sets the codec timebase (e.g. 1/25), it is
assumed correct due to codec_tb_matches_dts==1, even if it is not able
accurately represent timestamps at all (e.g. 1/50 intervals).

At least this sample shows the above issue:
http://samples.ffmpeg.org/V-codecs/h264/hdtv-interlaced/sky_720p_test_why-cant-i-overwrite.ts

So some checks need to be added to the patch to guard against above. Or
use some entirely different/better approach :)

I'd really appreciate someone more experienced looking at this issue,
but I'll take a further look at this myself later as well.


>  libavformat/avformat.h |    2 ++
>  libavformat/utils.c    |   37 ++++++++++++++++++++++++++-----------
>  2 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index 83289e4..cf3b8fd 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -627,6 +627,8 @@ typedef struct AVStream {
>          int64_t last_dts;
>          int64_t duration_gcd;
>          int duration_count;
> +        int codec_tb_matches_dts;
> +        double codec_tb_dur_error;
>          double duration_error[MAX_STD_TIMEBASES];
>          int64_t codec_info_duration;
>      } *info;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index d12bbc2..b044f4d 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -2189,15 +2189,20 @@ static int get_std_framerate(int i){
>   * MPEG-2 commonly misuses field repeat flags to store different framerates.
>   * And there are "variable" fps files this needs to detect as well.
>   */
> -static int tb_unreliable(AVCodecContext *c){
> -    if(   c->time_base.den >= 101L*c->time_base.num
> -       || c->time_base.den <    5L*c->time_base.num
> -/*       || c->codec_tag == AV_RL32("DIVX")
> -       || c->codec_tag == AV_RL32("XVID")*/
> -       || c->codec_id == CODEC_ID_MPEG2VIDEO
> -       || c->codec_id == CODEC_ID_H264
> -       )
> +static int tb_unreliable(AVStream *st){
> +    if(   st->codec->time_base.den >= 101L*st->codec->time_base.num
> +       || st->codec->time_base.den <    5L*st->codec->time_base.num)
>          return 1;
> +
> +    if(st->info->codec_tb_matches_dts)
> +        return 0;
> +
> +    if(/* st->codec->codec_tag == AV_RL32("DIVX")
> +       || st->codec->codec_tag == AV_RL32("XVID") ||*/
> +          st->codec->codec_id == CODEC_ID_MPEG2VIDEO
> +       || st->codec->codec_id == CODEC_ID_H264)
> +        return 1;
> +
>      return 0;
>  }
>  
> @@ -2271,7 +2276,7 @@ int av_find_stream_info(AVFormatContext *ic)
>              if (!has_codec_parameters(st->codec))
>                  break;
>              /* variable fps and no guess at the real fps */
> -            if(   tb_unreliable(st->codec) && !(st->r_frame_rate.num && st->avg_frame_rate.num)
> +            if(   tb_unreliable(st) && !(st->r_frame_rate.num && st->avg_frame_rate.num)
>                 && st->info->duration_count<20 && st->codec->codec_type == AVMEDIA_TYPE_VIDEO)
>                  break;
>              if(st->parser && st->parser->parser->split && !st->codec->extradata)
> @@ -2351,6 +2356,16 @@ int av_find_stream_info(AVFormatContext *ic)
>                      st->info->duration_error[i] += error*error;
>                  }
>                  st->info->duration_count++;
> +
> +                if (st->info->duration_count <= 4) {
> +                    double codec_tb_rate = st->codec->time_base.den / (double)st->codec->time_base.num * st->codec->ticks_per_frame;

And the above should be "/ st->codec->ticks_per_frame;", I think (didn't
look in detail right now).

> +                    double error = dur - lrintf(dur * codec_tb_rate) / codec_tb_rate;
> +                    st->info->codec_tb_dur_error += error*error;
> +
> +                    if (st->info->duration_count == 4 && st->info->codec_tb_dur_error * codec_tb_rate < 0.0001)
> +                        st->info->codec_tb_matches_dts = 1;
> +                }
> +
>                  // ignore the first 4 values, they might have some random jitter
>                  if (st->info->duration_count > 3)
>                      st->info->duration_gcd = av_gcd(st->info->duration_gcd, duration);
> @@ -2398,10 +2413,10 @@ int av_find_stream_info(AVFormatContext *ic)
>              // the check for tb_unreliable() is not completely correct, since this is not about handling
>              // a unreliable/inexact time base, but a time base that is finer than necessary, as e.g.
>              // ipmovie.c produces.
> -            if (tb_unreliable(st->codec) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num)
> +            if (tb_unreliable(st) && st->info->duration_count > 15 && st->info->duration_gcd > 1 && !st->r_frame_rate.num)
>                  av_reduce(&st->r_frame_rate.num, &st->r_frame_rate.den, st->time_base.den, st->time_base.num * st->info->duration_gcd, INT_MAX);
>              if (st->info->duration_count && !st->r_frame_rate.num
> -               && tb_unreliable(st->codec) /*&&
> +               && tb_unreliable(st) /*&&
>                 //FIXME we should not special-case MPEG-2, but this needs testing with non-MPEG-2 ...
>                 st->time_base.num*duration_sum[i]/st->info->duration_count*101LL > st->time_base.den*/){
>                  int num = 0;


-- 
Anssi Hannula