[FFmpeg-devel] [PATCH] lavf/mov.c: Guess video codec delay based on PTS while parsing MOV header.

[Sasi Inguva]

On Sun, Nov 19, 2017 at 1:17 AM, Michael Niedermayer <michael at niedermayer.cc
> wrote:

> On Sat, Nov 18, 2017 at 11:12:17AM -0800, Sasi Inguva wrote:
> > Signed-off-by: Sasi Inguva <isasi at google.com>
> > ---
> >  libavformat/mov.c                | 54 ++++++++++++++++++++++++++++++
> ++++++++++
> >  tests/fate/mov.mak               |  5 ++++
> >  tests/ref/fate/mov-guess-delay-1 |  3 +++
> >  tests/ref/fate/mov-guess-delay-2 |  3 +++
> >  4 files changed, 65 insertions(+)
> >  create mode 100644 tests/ref/fate/mov-guess-delay-1
> >  create mode 100644 tests/ref/fate/mov-guess-delay-2
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index fd170baa57..7354652c6e 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -3213,6 +3213,58 @@ static int64_t add_ctts_entry(MOVStts**
> ctts_data, unsigned int* ctts_count, uns
> >      return *ctts_count;
> >  }
> >
> > +static void mov_guess_video_delay(MOVContext *c, AVStream* st) {
> > +    MOVStreamContext *msc = st->priv_data;
> > +    int ind;
> > +    int ctts_ind = 0;
> > +    int ctts_sample = 0;
> > +    int64_t curr_pts = AV_NOPTS_VALUE;
> > +    int64_t prev_pts = AV_NOPTS_VALUE;
> > +    int64_t prev_max_pts = AV_NOPTS_VALUE;
> > +    int num_swaps = 0;
> > +
> > +    if (st->codecpar->video_delay <= 0 && msc->ctts_data &&
> > +        (st->codecpar->codec_id == AV_CODEC_ID_MPEG2VIDEO ||
> > +         st->codecpar->codec_id == AV_CODEC_ID_MPEG4 ||
> > +         st->codecpar->codec_id == AV_CODEC_ID_VC1 ||
> > +         st->codecpar->codec_id == AV_CODEC_ID_H263 ||
> > +         st->codecpar->codec_id == AV_CODEC_ID_H264 ||
> > +         st->codecpar->codec_id == AV_CODEC_ID_HEVC)) {
>
> Do all these cases really need this ?
>
> I just wanted to list all codecs with B-frames. H264, HEVC need it in
case, the bitstream_restriction_flag is not set in the bitstream. Other
codecs might need it only if the bitstream has been written incorrectly
i.e. writing 0 for the delay bit when there are B-frames etc. I can
restrict to H264 , HEVC if needed.

video_delay = 0 is also a valid value so it can be set to 0 already

intentionally
>
> I just want to avoid the case where the code incorrectly estimates a
video_delay that is lesser than what is written in the bitstream, because
that would be a fatal error resulting in incorrect timestamps. The other
case where the code estimates non-zero video_delay when it is zero in the
bitstream is benign.

>
>     > +        st->codecpar->video_delay = 0;
> > +        for(ind = 0; ind < st->nb_index_entries && ctts_ind <
> msc->ctts_count; ++ind) {
> > +            curr_pts = st->index_entries[ind].timestamp +
> msc->ctts_data[ctts_ind].duration;
> > +
>
> > +            // This is used as an indication that the previous GOP has
> ended and a
> > +            // new GOP has started.
>
> this is not neccesary a GOP uless i misread the code
>
> Corrected the comment.

>
> > +            if (curr_pts > prev_max_pts) {
> > +                st->codecpar->video_delay = FFMIN(FFMAX(st->codecpar->video_delay,
> num_swaps), 16);
> > +                num_swaps = 0;
> > +                prev_max_pts = curr_pts;
> > +            }
> > +
> > +            // Compute delay as the no. of "drop"s in PTS inside a GOP.
> > +            // Frames: I0 I1 B0 B1 B2
> > +            // PTS:     0  4  1  2  3 -> num_swaps = delay = 1 (4->1)
> > +            //
> > +            // Frames: I0 I1 B1 B0 B2
> > +            // PTS:     0  4  2  1  3 -> num_swaps = delay = 2 (4->2,
> 2->1)
>
> This may be incorret
>
> consider
> PTS    0  5  2  1  4  3
> BUFFER 0  05 25 25 45 45 5
> OUT          0  1  2  3  4  5
>
> So this is a delay = 2 but your code would set 3 i think
>
> True. Thanks for catching it. The issue will only get worse, with more no.
of B-frames in-between ( 0 7 2 1 4 3 6 4 -> would have delay 4 ) . I have
changed the code to estimate the delay as the length of the patch from max.
PTS to min. PTS . (So only 5->2->1 will be counted). Sending the new
patch.  PTAL. Thanks.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Those who are best at talking, realize last or never when they are wrong.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>