On Thu, Oct 6, 2016 at 4:08 PM, Hendrik Leppkes <h.lepp...@gmail.com> wrote: > On Tue, Oct 4, 2016 at 4:44 PM, James Almer <jamr...@gmail.com> wrote: >> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote: >>> On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfx...@googlemail.com> wrote: >>>> On Tue, 4 Oct 2016 14:15:03 +0200 >>>> Michael Niedermayer <mich...@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.lepp...@gmail.com> >>>>>> wrote: >>>>>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer >>>>>>> <mich...@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 >>>>>>>>> <mich...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
