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

[Hendrik Leppkes]

On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg at googlemail.com> wrote:
> 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.

And they don't go in there anymore right now, so thats that.

The real question is, what do we do about this merge now?
Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
considering it was unused in the current ABI/API, or would that be
considered an API break and we better delay this change until the next
major?

- Hendrik