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

[Hendrik Leppkes]

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.

- Hendrik