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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161004/cd0ff256/attachment.sig>