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

[Hendrik Leppkes]

On Thu, Oct 6, 2016 at 4:08 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Tue, Oct 4, 2016 at 4:44 PM, James Almer <jamrial at gmail.com> wrote:
>> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote:
>>> 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
>>
>> Delaying it could result in further merges becoming technically wrong,
>> or at least require extra manual changes for them to not misbehave in
>> our tree.
>>
>> IMO merge it now, and if needed/preferred, we could make sure it
>> doesn't make it to 3.2
>>
>
> Last call for any actual and clear objections to going forward with
> this route. I would like to get merging a bunch over the weekend so we
> get some progress here.
>

I applied the commit at the beginning of this thread and merged the
pkt_pts -> pts changes now.

- Hendrik