| Message ID | 20161001141545.9728-1-h.leppkes@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, Oct 1, 2016 at 4:15 PM, Hendrik Leppkes <h.leppkes@gmail.com> 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. > --- > ffmpeg.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/ffmpeg.c b/ffmpeg.c > index 9a8e65a..cdbf3d4 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output) > } > } > > - /* if the decoder provides a pts, use it instead of the last packet pts. > - the decoder could be delaying output by a packet or more. */ > - if (decoded_frame->pts != AV_NOPTS_VALUE) { > - ist->dts = ist->next_dts = ist->pts = ist->next_pts = av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q); > - decoded_frame_tb = avctx->time_base; > - } else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { > + if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { > decoded_frame->pts = decoded_frame->pkt_pts; > decoded_frame_tb = ist->st->time_base; > } else if (pkt->pts != AV_NOPTS_VALUE) { > -- Ping.
On Sat, 1 Oct 2016 16:15:45 +0200 Hendrik Leppkes <h.leppkes@gmail.com> 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. > --- > ffmpeg.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/ffmpeg.c b/ffmpeg.c > index 9a8e65a..cdbf3d4 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output) > } > } > > - /* if the decoder provides a pts, use it instead of the last packet pts. > - the decoder could be delaying output by a packet or more. */ > - if (decoded_frame->pts != AV_NOPTS_VALUE) { > - ist->dts = ist->next_dts = ist->pts = ist->next_pts = av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q); > - decoded_frame_tb = avctx->time_base; > - } else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { > + if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { > decoded_frame->pts = decoded_frame->pkt_pts; > decoded_frame_tb = ist->st->time_base; > } else if (pkt->pts != AV_NOPTS_VALUE) { Seems fine to me. No decoder should ever have set the pts field before. (Forgot to reply earlier.)
On Sun, Oct 2, 2016 at 6:22 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Sat, Oct 1, 2016 at 4:15 PM, Hendrik Leppkes <h.leppkes@gmail.com> 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. >> --- >> ffmpeg.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/ffmpeg.c b/ffmpeg.c >> index 9a8e65a..cdbf3d4 100644 >> --- a/ffmpeg.c >> +++ b/ffmpeg.c >> @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output) >> } >> } >> >> - /* if the decoder provides a pts, use it instead of the last packet pts. >> - the decoder could be delaying output by a packet or more. */ >> - if (decoded_frame->pts != AV_NOPTS_VALUE) { >> - ist->dts = ist->next_dts = ist->pts = ist->next_pts = av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q); >> - decoded_frame_tb = avctx->time_base; >> - } else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { >> + if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { >> decoded_frame->pts = decoded_frame->pkt_pts; >> decoded_frame_tb = ist->st->time_base; >> } else if (pkt->pts != AV_NOPTS_VALUE) { >> -- > > Ping. Last chance for further comments, otherwise I'll apply in the morning (~8 hours from now), so that merges can continue. - Hendrik
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. [...]
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). - Hendrik
On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes <h.leppkes@gmail.com> 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). > Of course that could still mean that some other apps "copied" the ffmpeg code and try to read this field - but is this a scenario we can really control? The pts field in AVFrame is currently unused for decoding, nothing sets it (except cuvid and openh264 or so, but those set it the same way it would be set in the future, so no changes there), ffmpeg.c trying to read it is a remnant from a long time ago (quick blame pins it at 2012 when decoding was changed to decode_audio4). I couldn't actually confirm if at that time (audio) decoders did even set AVFrame.pts, considering pkt_pts already existed then. So is starting to set a field that was previously (at least through 2 or so major bumps) unused a API break? Its always possible some app still tries to read it, but because its never set it didn't cause any problems so far. The alternative is of course to keep using pkt_pts, and keep pts unused for decoding, but I'm not entirely convinced there is a break here. - Hendrik
On Tue, Oct 04, 2016 at 10:30:24AM +0200, Hendrik Leppkes wrote: > On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes <h.leppkes@gmail.com> 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). > > > > Of course that could still mean that some other apps "copied" the > ffmpeg code and try to read this field - but is this a scenario we can > really control? > > The pts field in AVFrame is currently unused for decoding, nothing > sets it (except cuvid and openh264 or so, but those set it the same > way it would be set in the future, so no changes there), ffmpeg.c > trying to read it is a remnant from a long time ago (quick blame pins > it at 2012 when decoding was changed to decode_audio4). > I couldn't actually confirm if at that time (audio) decoders did even > set AVFrame.pts, considering pkt_pts already existed then. > > So is starting to set a field that was previously (at least through 2 > or so major bumps) unused a API break? > Its always possible some app still tries to read it, but because its > never set it didn't cause any problems so far. > > The alternative is of course to keep using pkt_pts, and keep pts > unused for decoding, but I'm not entirely convinced there is a break > here. not stating any oppinion in this paragraph but If use of AVFrame.pts is considered a bug in the current API then past releases with that API need to be fixed. If they are not fixed testing API/ABI for 3.2 will blow up (i havnt tried it yet for 3.2 but i did for past releases previously and mixing libs between releases and ffmpeg is required to not blow up, it protects against API/ABI breaks somewhat) Also release notes for 3.2 would be needed as current 3.1 would not mix well with a release with same soname and differently used AVFrame.pts. That is unless i miss something Somewhat off topic and my personal oppinion I think independant of field names and API, there are 3 or 4 types of timestamps, it would be good if user applications have some easy way of accessing all of them for a frame from a decoder. the 4 types are, * input AVPacket.pts based * input AVPacket.dts based * what is stored in the codec bitstream if any * some easy to use one that simple apps can just use and not need to worry about anything, aka one that is "correct" in almost all cases And of course i want the next release to work for distros and not see complaints about it causing pain due to API/ABI stuff, breaking packages and so on. And i want a clean, easy to use, maintainable and yet powerfull API About the original patch in this thread itself i have no real oppinon [...]
On Tue, Oct 4, 2016 at 1:55 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Oct 04, 2016 at 10:30:24AM +0200, Hendrik Leppkes wrote: >> On Tue, Oct 4, 2016 at 8:41 AM, Hendrik Leppkes <h.leppkes@gmail.com> 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). >> > >> >> Of course that could still mean that some other apps "copied" the >> ffmpeg code and try to read this field - but is this a scenario we can >> really control? >> >> The pts field in AVFrame is currently unused for decoding, nothing >> sets it (except cuvid and openh264 or so, but those set it the same >> way it would be set in the future, so no changes there), ffmpeg.c >> trying to read it is a remnant from a long time ago (quick blame pins >> it at 2012 when decoding was changed to decode_audio4). >> I couldn't actually confirm if at that time (audio) decoders did even >> set AVFrame.pts, considering pkt_pts already existed then. >> >> So is starting to set a field that was previously (at least through 2 >> or so major bumps) unused a API break? >> Its always possible some app still tries to read it, but because its >> never set it didn't cause any problems so far. >> >> The alternative is of course to keep using pkt_pts, and keep pts >> unused for decoding, but I'm not entirely convinced there is a break >> here. > > not stating any oppinion in this paragraph but > If use of AVFrame.pts is considered a bug in the current API then past > releases with that API need to be fixed. If they are not fixed > testing API/ABI for 3.2 will blow up (i havnt tried it yet for 3.2 > but i did for past releases previously and mixing libs between releases > and ffmpeg is required to not blow up, it protects against API/ABI > breaks somewhat) > Also release notes for 3.2 would be needed as current 3.1 would not > mix well with a release with same soname and differently used > AVFrame.pts. That is unless i miss something > > Somewhat off topic and my personal oppinion > I think independant of field names and API, there are 3 or 4 types of > timestamps, it would be good if user applications have some easy > way of accessing all of them for a frame from a decoder. > the 4 types are, > * input AVPacket.pts based > * input AVPacket.dts based > * what is stored in the codec bitstream if any > * some easy to use one that simple apps can just use and not need to > worry about anything, aka one that is "correct" in almost all cases > Well we have 3 of those, and nothing that provides the codec timestamps (not sure they would even exist anywhere). If the 4th kind is added, it should definitely not be in AVFrame.pts however, since people might mistakenly use that as a general purpose timestamp. The entire reason for the upcoming change is simple: - AVFrame.pts is used for the timestamp in filtering and encoding, but unused in decoding, which is inconsistent - AVFrame.pkt_pts is only used for decoding, unused anywhere else. So combining those into one field seems like a logical step. AVFrame.pts has been unused for decoding for ~3 years (including several major bumps), and even before that its use was probably not very useful. Is that enough time to recycle it? Or should we rather skip the change and possibly queue it for later (ie. the next bump)? - Hendrik
diff --git a/ffmpeg.c b/ffmpeg.c index 9a8e65a..cdbf3d4 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -2058,12 +2058,7 @@ static int decode_audio(InputStream *ist, AVPacket *pkt, int *got_output) } } - /* if the decoder provides a pts, use it instead of the last packet pts. - the decoder could be delaying output by a packet or more. */ - if (decoded_frame->pts != AV_NOPTS_VALUE) { - ist->dts = ist->next_dts = ist->pts = ist->next_pts = av_rescale_q(decoded_frame->pts, avctx->time_base, AV_TIME_BASE_Q); - decoded_frame_tb = avctx->time_base; - } else if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { + if (decoded_frame->pkt_pts != AV_NOPTS_VALUE) { decoded_frame->pts = decoded_frame->pkt_pts; decoded_frame_tb = ist->st->time_base; } else if (pkt->pts != AV_NOPTS_VALUE) {