[FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

[wm4]

On Tue, 4 Oct 2016 14:15:03 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
> > On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:  
> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:  
> > >> On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:  
> > >>> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
> > >>> <michael at niedermayer.cc> wrote:  
> > >>> > On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:  
> > >>> >> Decoders have previously not used AVFrame.pts, and with the upcoming
> > >>> >> deprecation of pkt_pts (in favor of pts), this would lead to an errorneous
> > >>> >> interpration of timestamps.  
> > >>> >
> > >>> > I probably misunderstand the commit message but
> > >>> > If code is changed in a user application that cannot really lift
> > >>> > some blockage from changing a lib.
> > >>> > a lib can only change in an incompaible way with (deprecation and)
> > >>> > major version bump.
> > >>> >  
> > >>>
> > >>> The lib never did what this code suggests it did, not that I remember
> > >>> (so at least not for a long long time).  
> > >>
> > >> release/2.0 with
> > >>
> > >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > >> index 29d5492..57c8d50 100644
> > >> --- a/libavcodec/utils.c
> > >> +++ b/libavcodec/utils.c
> > >> @@ -2008,7 +2008,7 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
> > >>                  avci->to_free.extended_data = avci->to_free.data;
> > >>                  memset(picture->buf, 0, sizeof(picture->buf));
> > >>              }
> > >> -
> > >> +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
> > >>              avctx->frame_number++;
> > >>              av_frame_set_best_effort_timestamp(picture,
> > >>                                                 guess_correct_pts(avctx,
> > >>
> > >> causes many tests to fail, indicating that AVFrame.pts was set for
> > >> several video decoders, the ffmpeg code is audio decoder specific
> > >> and i failed to find a case where it was triggered, i tried IIRC 3
> > >> or so checkouts from the past
> > >>
> > >> so AVFrame.pts was maybe never set for decoding audio but it was set
> > >> for video  
> > >
> > > Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
> > > Because thats what it would be set to after the merge. A quick check
> > > in the 2.0 code base looks like some decoders did set that, but to the
> > > exact same value as pkt_pts (which is what the merge is proposing
> > > right now as well)
> > >  
> > 
> > And I found this (after 2.0):
> > http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
> > 
> > Which apparently set pts for mpeg4 to a number parsed from the
> > bitstream, entirely uncorrelated to container or audio timestamps, so
> > using them would have been rather impractical for any real use-cases.  
> 
> They can be usfull, some random examples:
> 
> playing MPEG4-ES with timing stored from the bitstream (assuming there
>   is no demuxer lib used that is capable to extract them)
> 
> forensics, raw video stream could have its timing
>   recovered, a video with manipulated container timestamps could be
>   detected.
> 
> error correction, if the container level timestamps are lost or
>   corrupted the stream level ones can be used to recreate them
> 
> There may be more, these are just some of the top of my head,
> not your mainstream multimedia player stuff maybe but they arent
> useless
> 
> [...]
> 

They don't belong into the AVFrame.pts field, though.