| Message ID | 20161004112348.GB5270@nb4 |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer <michael@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@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) - Hendrik
On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer > <michael@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@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
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@gmail.com> wrote: > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer > > <michael@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@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 [...]
On Tue, 4 Oct 2016 14:15:03 +0200 Michael Niedermayer <michael@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@gmail.com> wrote: > > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer > > > <michael@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@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.
On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 4 Oct 2016 14:15:03 +0200 > Michael Niedermayer <michael@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@gmail.com> wrote: >> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer >> > > <michael@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@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
On Tue, 4 Oct 2016 16:35:02 +0200 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg@googlemail.com> wrote: > > On Tue, 4 Oct 2016 14:15:03 +0200 > > Michael Niedermayer <michael@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@gmail.com> wrote: > >> > > On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer > >> > > <michael@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@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? IMO applications which did this were pretty broken anyway, and we should be able to get away with a simple version bump.
On 10/4/2016 11:35 AM, Hendrik Leppkes wrote: > On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg@googlemail.com> wrote: >> On Tue, 4 Oct 2016 14:15:03 +0200 >> Michael Niedermayer <michael@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@gmail.com> wrote: >>>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer >>>>> <michael@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@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
On Tue, Oct 4, 2016 at 4:44 PM, James Almer <jamrial@gmail.com> wrote: > On 10/4/2016 11:35 AM, Hendrik Leppkes wrote: >> On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg@googlemail.com> wrote: >>> On Tue, 4 Oct 2016 14:15:03 +0200 >>> Michael Niedermayer <michael@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@gmail.com> wrote: >>>>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer >>>>>> <michael@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@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. - Hendrik
On Thu, Oct 6, 2016 at 4:08 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Oct 4, 2016 at 4:44 PM, James Almer <jamrial@gmail.com> wrote: >> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote: >>> On Tue, Oct 4, 2016 at 4:32 PM, wm4 <nfxjfg@googlemail.com> wrote: >>>> On Tue, 4 Oct 2016 14:15:03 +0200 >>>> Michael Niedermayer <michael@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@gmail.com> wrote: >>>>>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer >>>>>>> <michael@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@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
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,