| Message ID | 20171109122124.088A73CA@melkor.jcowgill.uk |
|---|---|
| State | Accepted |
| Commit | 02ba4b91b5616ecbebee5c9565e1be7af2a6b980 |
| Headers | show |
On 11/9/2017 12:21 PM, James Cowgill wrote: > + if (avci->draining_done && pkt && pkt->size != 0) { > + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); > + avcodec_flush_buffers(avctx); > + } Is it more sensible to actually return an error here? Otherwise it's just enabling future incorrect code; API users tend to ignore stderr warnings. I guess an argument could be made both ways. - Derek
On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> wrote: > In commit 061a0c14bb57 ("decode: restructure the core decoding code"), the > deprecated avcodec_decode_* APIs were reworked so that they called into the > new avcodec_send_packet / avcodec_receive_frame API. This had the side effect > of prohibiting sending new packets containing data after a drain > packet, but in previous versions of FFmpeg this "worked" and some > applications relied on it. > > To restore some compatibility, reset the codec if we receive a new non-drain > packet using the old API after draining has completed. While this does > not give the same behaviour as the old API did, in the majority of cases > it works and it does not require changes to any other part of the decoding > code. > > Fixes ticket #6775 > Signed-off-by: James Cowgill <jcowgill@debian.org> > --- > libavcodec/decode.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavcodec/decode.c b/libavcodec/decode.c > index 86fe5aef52..2f1932fa85 100644 > --- a/libavcodec/decode.c > +++ b/libavcodec/decode.c > @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, > > av_assert0(avci->compat_decode_consumed == 0); > > + if (avci->draining_done && pkt && pkt->size != 0) { > + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); > + avcodec_flush_buffers(avctx); > + } > + I don't think this is a good idea. Draining and not flushing afterwards is a bug in the calling code, and even before recent changes it would result in inconsistent behavior and even crashes (with select decoders). - Hendrik
Hi, On 09/11/17 14:02, Hendrik Leppkes wrote: > On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> wrote: >> In commit 061a0c14bb57 ("decode: restructure the core decoding code"), the >> deprecated avcodec_decode_* APIs were reworked so that they called into the >> new avcodec_send_packet / avcodec_receive_frame API. This had the side effect >> of prohibiting sending new packets containing data after a drain >> packet, but in previous versions of FFmpeg this "worked" and some >> applications relied on it. >> >> To restore some compatibility, reset the codec if we receive a new non-drain >> packet using the old API after draining has completed. While this does >> not give the same behaviour as the old API did, in the majority of cases >> it works and it does not require changes to any other part of the decoding >> code. >> >> Fixes ticket #6775 >> Signed-off-by: James Cowgill <jcowgill@debian.org> >> --- >> libavcodec/decode.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >> index 86fe5aef52..2f1932fa85 100644 >> --- a/libavcodec/decode.c >> +++ b/libavcodec/decode.c >> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, >> >> av_assert0(avci->compat_decode_consumed == 0); >> >> + if (avci->draining_done && pkt && pkt->size != 0) { >> + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); >> + avcodec_flush_buffers(avctx); >> + } >> + > > I don't think this is a good idea. Draining and not flushing > afterwards is a bug in the calling code, and even before recent > changes it would result in inconsistent behavior and even crashes > (with select decoders). I am fully aware that this will only trigger if the calling code is buggy. I am trying to avoid silent breakage of those applications doing this when upgrading to ffmpeg 3.4. I was looking at the documentation of avcodec_decode_* recently because of this and I had some trouble deciding if using the API this way was incorrect. I expect the downstreams affected thought that what they were doing was fine and then got angry when ffmpeg suddenly "broke" their code. This patch at least allows some sort of "transitional period" until downstreams update. From the perspective of Debian, I could either apply this patch to ffmpeg, or I would have to go through over 100 reverse dependencies to see if they abuse the API and then fix them. I currently know of two (gst-libav1.0 and kodi), but there could be more - especially within less used packages. Thanks, James
On Thu, 9 Nov 2017, James Cowgill wrote: > Hi, > > On 09/11/17 14:02, Hendrik Leppkes wrote: >> On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> wrote: >>> In commit 061a0c14bb57 ("decode: restructure the core decoding code"), the >>> deprecated avcodec_decode_* APIs were reworked so that they called into the >>> new avcodec_send_packet / avcodec_receive_frame API. This had the side effect >>> of prohibiting sending new packets containing data after a drain >>> packet, but in previous versions of FFmpeg this "worked" and some >>> applications relied on it. >>> >>> To restore some compatibility, reset the codec if we receive a new non-drain >>> packet using the old API after draining has completed. While this does >>> not give the same behaviour as the old API did, in the majority of cases >>> it works and it does not require changes to any other part of the decoding >>> code. >>> >>> Fixes ticket #6775 >>> Signed-off-by: James Cowgill <jcowgill@debian.org> >>> --- >>> libavcodec/decode.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>> index 86fe5aef52..2f1932fa85 100644 >>> --- a/libavcodec/decode.c >>> +++ b/libavcodec/decode.c >>> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, >>> >>> av_assert0(avci->compat_decode_consumed == 0); >>> >>> + if (avci->draining_done && pkt && pkt->size != 0) { >>> + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); >>> + avcodec_flush_buffers(avctx); >>> + } >>> + >> >> I don't think this is a good idea. Draining and not flushing >> afterwards is a bug in the calling code, and even before recent >> changes it would result in inconsistent behavior and even crashes >> (with select decoders). > > I am fully aware that this will only trigger if the calling code is > buggy. I am trying to avoid silent breakage of those applications doing > this when upgrading to ffmpeg 3.4. > > I was looking at the documentation of avcodec_decode_* recently because > of this and I had some trouble deciding if using the API this way was > incorrect. I expect the downstreams affected thought that what they were > doing was fine and then got angry when ffmpeg suddenly "broke" their > code. This patch at least allows some sort of "transitional period" > until downstreams update. I think the intent was to flush the codec by passing the NULL packets to it, so it makes a lot of sense to actually do that. Especially since by implicitly doing a flush, we can avoid the undefined behaviour/crashes on the codec side. Also this is only compatibility code, which probably will be removed at the next bump, I see no harm in making it as compatible as realistically possible. Regards, Marton
On Tue, Nov 21, 2017 at 10:48:19PM +0100, Marton Balint wrote: > > > On Thu, 9 Nov 2017, James Cowgill wrote: > > >Hi, > > > >On 09/11/17 14:02, Hendrik Leppkes wrote: > >>On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> wrote: > >>>In commit 061a0c14bb57 ("decode: restructure the core decoding code"), the > >>>deprecated avcodec_decode_* APIs were reworked so that they called into the > >>>new avcodec_send_packet / avcodec_receive_frame API. This had the side effect > >>>of prohibiting sending new packets containing data after a drain > >>>packet, but in previous versions of FFmpeg this "worked" and some > >>>applications relied on it. > >>> > >>>To restore some compatibility, reset the codec if we receive a new non-drain > >>>packet using the old API after draining has completed. While this does > >>>not give the same behaviour as the old API did, in the majority of cases > >>>it works and it does not require changes to any other part of the decoding > >>>code. > >>> > >>>Fixes ticket #6775 > >>>Signed-off-by: James Cowgill <jcowgill@debian.org> > >>>--- > >>> libavcodec/decode.c | 5 +++++ > >>> 1 file changed, 5 insertions(+) > >>> > >>>diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >>>index 86fe5aef52..2f1932fa85 100644 > >>>--- a/libavcodec/decode.c > >>>+++ b/libavcodec/decode.c > >>>@@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, > >>> > >>> av_assert0(avci->compat_decode_consumed == 0); > >>> > >>>+ if (avci->draining_done && pkt && pkt->size != 0) { > >>>+ av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); > >>>+ avcodec_flush_buffers(avctx); > >>>+ } > >>>+ > >> > >>I don't think this is a good idea. Draining and not flushing > >>afterwards is a bug in the calling code, and even before recent > >>changes it would result in inconsistent behavior and even crashes > >>(with select decoders). > > > >I am fully aware that this will only trigger if the calling code is > >buggy. I am trying to avoid silent breakage of those applications doing > >this when upgrading to ffmpeg 3.4. > > > >I was looking at the documentation of avcodec_decode_* recently because > >of this and I had some trouble deciding if using the API this way was > >incorrect. I expect the downstreams affected thought that what they were > >doing was fine and then got angry when ffmpeg suddenly "broke" their > >code. This patch at least allows some sort of "transitional period" > >until downstreams update. > > I think the intent was to flush the codec by passing the NULL > packets to it, so it makes a lot of sense to actually do that. > Especially since by implicitly doing a flush, we can avoid the > undefined behaviour/crashes on the codec side. > > Also this is only compatibility code, which probably will be removed > at the next bump, I see no harm in making it as compatible as > realistically possible. i agree and i would appreciate if this gets resolved one way or another so i can make the release Thanks [...]
On 11/21/2017 6:48 PM, Marton Balint wrote: > > > On Thu, 9 Nov 2017, James Cowgill wrote: > >> Hi, >> >> On 09/11/17 14:02, Hendrik Leppkes wrote: >>> On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> >>> wrote: >>>> In commit 061a0c14bb57 ("decode: restructure the core decoding >>>> code"), the >>>> deprecated avcodec_decode_* APIs were reworked so that they called >>>> into the >>>> new avcodec_send_packet / avcodec_receive_frame API. This had the >>>> side effect >>>> of prohibiting sending new packets containing data after a drain >>>> packet, but in previous versions of FFmpeg this "worked" and some >>>> applications relied on it. >>>> >>>> To restore some compatibility, reset the codec if we receive a new >>>> non-drain >>>> packet using the old API after draining has completed. While this does >>>> not give the same behaviour as the old API did, in the majority of >>>> cases >>>> it works and it does not require changes to any other part of the >>>> decoding >>>> code. >>>> >>>> Fixes ticket #6775 >>>> Signed-off-by: James Cowgill <jcowgill@debian.org> >>>> --- >>>> libavcodec/decode.c | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>> index 86fe5aef52..2f1932fa85 100644 >>>> --- a/libavcodec/decode.c >>>> +++ b/libavcodec/decode.c >>>> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, >>>> AVFrame *frame, >>>> >>>> av_assert0(avci->compat_decode_consumed == 0); >>>> >>>> + if (avci->draining_done && pkt && pkt->size != 0) { >>>> + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after >>>> EOF\n"); >>>> + avcodec_flush_buffers(avctx); >>>> + } >>>> + >>> >>> I don't think this is a good idea. Draining and not flushing >>> afterwards is a bug in the calling code, and even before recent >>> changes it would result in inconsistent behavior and even crashes >>> (with select decoders). >> >> I am fully aware that this will only trigger if the calling code is >> buggy. I am trying to avoid silent breakage of those applications doing >> this when upgrading to ffmpeg 3.4. >> >> I was looking at the documentation of avcodec_decode_* recently because >> of this and I had some trouble deciding if using the API this way was >> incorrect. I expect the downstreams affected thought that what they were >> doing was fine and then got angry when ffmpeg suddenly "broke" their >> code. This patch at least allows some sort of "transitional period" >> until downstreams update. > > I think the intent was to flush the codec by passing the NULL packets to > it, so it makes a lot of sense to actually do that. Especially since by > implicitly doing a flush, we can avoid the undefined behaviour/crashes > on the codec side. > > Also this is only compatibility code, which probably will be removed at > the next bump, I see no harm in making it as compatible as realistically > possible. The old decode API is not scheduled for removal right now probably because 99% of decoders need to be ported. This compat code was written so the old API becomes a wrapper for the new rather than the other way around, as it was up to 3.3. Supposedly a good portion of the versatility of the new API would be handicapped otherwise. Personally, I think this should be left as is. It is a good incentive for downstream to migrate to the new API, as they technically were misusing the old API to begin with. Between fixing their old API usage and migrating, the choice should be obvious.
James Almer (2017-11-26): > The old decode API is not scheduled for removal right now probably > because 99% of decoders need to be ported. I think this statement contains some confusion that is harmful to the discussion. There are two interfaces worth considering in this discussion: the application -> library interface, i.e. the avcodec_decode_*dio() functions, and the framework -> decoder interface, i.e. the decode / receive_frame / ... callbacks. When you are stating "because 99% of decoders need to be ported", you are referring to the framework-decoder interface. On the other hand, the misuse of the API that is at the origin of this thread is related to the application-library interface. We could deprecate avcodec_decode_*dio() right now even though the decoders are not all ported. Regards,
On 11/26/2017 12:19 PM, Nicolas George wrote: > James Almer (2017-11-26): >> The old decode API is not scheduled for removal right now probably >> because 99% of decoders need to be ported. > > I think this statement contains some confusion that is harmful to the > discussion. > > There are two interfaces worth considering in this discussion: the > application -> library interface, i.e. the avcodec_decode_*dio() > functions, and the framework -> decoder interface, i.e. the decode / > receive_frame / ... callbacks. > > When you are stating "because 99% of decoders need to be ported", you > are referring to the framework-decoder interface. On the other hand, the > misuse of the API that is at the origin of this thread is related to the > application-library interface. Yes, my bad. Got the public API and the internal callbacks mixed. So ignore that part. Guess then that the functions did not get a removal schedule because they are still too ubiquitous downstream. My second paragraph stands, in any case. I consider this a good chance to get downstreams to migrate. > > We could deprecate avcodec_decode_*dio() right now even though the > decoders are not all ported. They are already deprecated. I assume you meant remove. > > Regards, > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Sun, 26 Nov 2017, James Almer wrote: > On 11/26/2017 12:19 PM, Nicolas George wrote: >> James Almer (2017-11-26): >>> The old decode API is not scheduled for removal right now probably >>> because 99% of decoders need to be ported. >> >> I think this statement contains some confusion that is harmful to the >> discussion. >> >> There are two interfaces worth considering in this discussion: the >> application -> library interface, i.e. the avcodec_decode_*dio() >> functions, and the framework -> decoder interface, i.e. the decode / >> receive_frame / ... callbacks. >> >> When you are stating "because 99% of decoders need to be ported", you >> are referring to the framework-decoder interface. On the other hand, the >> misuse of the API that is at the origin of this thread is related to the >> application-library interface. > > Yes, my bad. Got the public API and the internal callbacks mixed. So > ignore that part. > Guess then that the functions did not get a removal schedule because > they are still too ubiquitous downstream. > > My second paragraph stands, in any case. I consider this a good chance > to get downstreams to migrate. Okay, I am exagarating a bit, but unconditionally returning AVERROR(ENOSYS) would be an even better incentive, no? :) We can blame API usage (we should rather blame unclear documentation), but no matter how we put it, with the change, we broke the user experience of two major projects. If fixing it (at least partially) is so easy, I still don't see why we should not do that. People who still oppose this change, please respond. Thanks, Marton
Marton Balint (2017-11-26): > Okay, I am exagarating a bit, but unconditionally returning AVERROR(ENOSYS) > would be an even better incentive, no? :) For invalid uses of the API that can be easily avoided by the application (like not explicitly passing NULL to a function), a hard crash is even better. Regards,
On Sun, Nov 26, 2017 at 12:09:35PM -0300, James Almer wrote: > On 11/21/2017 6:48 PM, Marton Balint wrote: > > > > > > On Thu, 9 Nov 2017, James Cowgill wrote: > > > >> Hi, > >> > >> On 09/11/17 14:02, Hendrik Leppkes wrote: > >>> On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> > >>> wrote: > >>>> In commit 061a0c14bb57 ("decode: restructure the core decoding > >>>> code"), the > >>>> deprecated avcodec_decode_* APIs were reworked so that they called > >>>> into the > >>>> new avcodec_send_packet / avcodec_receive_frame API. This had the > >>>> side effect > >>>> of prohibiting sending new packets containing data after a drain > >>>> packet, but in previous versions of FFmpeg this "worked" and some > >>>> applications relied on it. > >>>> > >>>> To restore some compatibility, reset the codec if we receive a new > >>>> non-drain > >>>> packet using the old API after draining has completed. While this does > >>>> not give the same behaviour as the old API did, in the majority of > >>>> cases > >>>> it works and it does not require changes to any other part of the > >>>> decoding > >>>> code. > >>>> > >>>> Fixes ticket #6775 > >>>> Signed-off-by: James Cowgill <jcowgill@debian.org> > >>>> --- > >>>> libavcodec/decode.c | 5 +++++ > >>>> 1 file changed, 5 insertions(+) > >>>> > >>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >>>> index 86fe5aef52..2f1932fa85 100644 > >>>> --- a/libavcodec/decode.c > >>>> +++ b/libavcodec/decode.c > >>>> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, > >>>> AVFrame *frame, > >>>> > >>>> av_assert0(avci->compat_decode_consumed == 0); > >>>> > >>>> + if (avci->draining_done && pkt && pkt->size != 0) { > >>>> + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after > >>>> EOF\n"); > >>>> + avcodec_flush_buffers(avctx); > >>>> + } > >>>> + > >>> > >>> I don't think this is a good idea. Draining and not flushing > >>> afterwards is a bug in the calling code, and even before recent > >>> changes it would result in inconsistent behavior and even crashes > >>> (with select decoders). > >> > >> I am fully aware that this will only trigger if the calling code is > >> buggy. I am trying to avoid silent breakage of those applications doing > >> this when upgrading to ffmpeg 3.4. > >> > >> I was looking at the documentation of avcodec_decode_* recently because > >> of this and I had some trouble deciding if using the API this way was > >> incorrect. I expect the downstreams affected thought that what they were > >> doing was fine and then got angry when ffmpeg suddenly "broke" their > >> code. This patch at least allows some sort of "transitional period" > >> until downstreams update. > > > > I think the intent was to flush the codec by passing the NULL packets to > > it, so it makes a lot of sense to actually do that. Especially since by > > implicitly doing a flush, we can avoid the undefined behaviour/crashes > > on the codec side. > > > > Also this is only compatibility code, which probably will be removed at > > the next bump, I see no harm in making it as compatible as realistically > > possible. > > The old decode API is not scheduled for removal right now probably > because 99% of decoders need to be ported. > This compat code was written so the old API becomes a wrapper for the > new rather than the other way around, as it was up to 3.3. Supposedly a > good portion of the versatility of the new API would be handicapped > otherwise. > > Personally, I think this should be left as is. It is a good incentive > for downstream to migrate to the new API, as they technically were > misusing the old API to begin with. providing compatibility support for an old API that does not actually work with how applications used the old API is something a tad bit bizzare We want to minimize the work everyone has to do. The more time people have, the more they can spend on improving free software. If the old API is going to be removed, any work people have to do to hunt and track implementation changes in our old API the more they have wasted time. If you want people to spend their time on the new API, then you should not introduce issues in the old API that they need to workaround They that way just lost time (debug, fix, test) they could have spend on the new API or on anything else > Between fixing their old API usage and migrating, the choice should be > obvious. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, 27 Nov 2017, Michael Niedermayer wrote: > On Sun, Nov 26, 2017 at 12:09:35PM -0300, James Almer wrote: >> On 11/21/2017 6:48 PM, Marton Balint wrote: >>> >>> >>> On Thu, 9 Nov 2017, James Cowgill wrote: >>> >>>> Hi, >>>> >>>> On 09/11/17 14:02, Hendrik Leppkes wrote: >>>>> On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> >>>>> wrote: >>>>>> In commit 061a0c14bb57 ("decode: restructure the core decoding >>>>>> code"), the >>>>>> deprecated avcodec_decode_* APIs were reworked so that they called >>>>>> into the >>>>>> new avcodec_send_packet / avcodec_receive_frame API. This had the >>>>>> side effect >>>>>> of prohibiting sending new packets containing data after a drain >>>>>> packet, but in previous versions of FFmpeg this "worked" and some >>>>>> applications relied on it. >>>>>> >>>>>> To restore some compatibility, reset the codec if we receive a new >>>>>> non-drain >>>>>> packet using the old API after draining has completed. While this does >>>>>> not give the same behaviour as the old API did, in the majority of >>>>>> cases >>>>>> it works and it does not require changes to any other part of the >>>>>> decoding >>>>>> code. >>>>>> >>>>>> Fixes ticket #6775 >>>>>> Signed-off-by: James Cowgill <jcowgill@debian.org> >>>>>> --- >>>>>> libavcodec/decode.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c >>>>>> index 86fe5aef52..2f1932fa85 100644 >>>>>> --- a/libavcodec/decode.c >>>>>> +++ b/libavcodec/decode.c >>>>>> @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, >>>>>> AVFrame *frame, >>>>>> >>>>>> av_assert0(avci->compat_decode_consumed == 0); >>>>>> >>>>>> + if (avci->draining_done && pkt && pkt->size != 0) { >>>>>> + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after >>>>>> EOF\n"); >>>>>> + avcodec_flush_buffers(avctx); >>>>>> + } >>>>>> + >>>>> >>>>> I don't think this is a good idea. Draining and not flushing >>>>> afterwards is a bug in the calling code, and even before recent >>>>> changes it would result in inconsistent behavior and even crashes >>>>> (with select decoders). >>>> >>>> I am fully aware that this will only trigger if the calling code is >>>> buggy. I am trying to avoid silent breakage of those applications doing >>>> this when upgrading to ffmpeg 3.4. >>>> >>>> I was looking at the documentation of avcodec_decode_* recently because >>>> of this and I had some trouble deciding if using the API this way was >>>> incorrect. I expect the downstreams affected thought that what they were >>>> doing was fine and then got angry when ffmpeg suddenly "broke" their >>>> code. This patch at least allows some sort of "transitional period" >>>> until downstreams update. >>> >>> I think the intent was to flush the codec by passing the NULL packets to >>> it, so it makes a lot of sense to actually do that. Especially since by >>> implicitly doing a flush, we can avoid the undefined behaviour/crashes >>> on the codec side. >>> >>> Also this is only compatibility code, which probably will be removed at >>> the next bump, I see no harm in making it as compatible as realistically >>> possible. >> >> The old decode API is not scheduled for removal right now probably >> because 99% of decoders need to be ported. >> This compat code was written so the old API becomes a wrapper for the >> new rather than the other way around, as it was up to 3.3. Supposedly a >> good portion of the versatility of the new API would be handicapped >> otherwise. >> > >> Personally, I think this should be left as is. It is a good incentive >> for downstream to migrate to the new API, as they technically were >> misusing the old API to begin with. > > providing compatibility support for an old API that does not actually > work with how applications used the old API is something a tad bit > bizzare > > We want to minimize the work everyone has to do. > The more time people have, the more they can spend on improving free > software. > > If the old API is going to be removed, any work people have to do > to hunt and track implementation changes in our old API the more > they have wasted time. > If you want people to spend their time on the new API, then you > should not introduce issues in the old API that they need to > workaround > They that way just lost time (debug, fix, test) they could have spend > on the new API or on anything else > Applied and backported. Regards, Marton
On Sat, Dec 09, 2017 at 09:46:53PM +0100, Marton Balint wrote: > > > On Mon, 27 Nov 2017, Michael Niedermayer wrote: > > >On Sun, Nov 26, 2017 at 12:09:35PM -0300, James Almer wrote: > >>On 11/21/2017 6:48 PM, Marton Balint wrote: > >>> > >>> > >>>On Thu, 9 Nov 2017, James Cowgill wrote: > >>> > >>>>Hi, > >>>> > >>>>On 09/11/17 14:02, Hendrik Leppkes wrote: > >>>>>On Thu, Nov 9, 2017 at 1:21 PM, James Cowgill <jcowgill@debian.org> > >>>>>wrote: > >>>>>>In commit 061a0c14bb57 ("decode: restructure the core decoding > >>>>>>code"), the > >>>>>>deprecated avcodec_decode_* APIs were reworked so that they called > >>>>>>into the > >>>>>>new avcodec_send_packet / avcodec_receive_frame API. This had the > >>>>>>side effect > >>>>>>of prohibiting sending new packets containing data after a drain > >>>>>>packet, but in previous versions of FFmpeg this "worked" and some > >>>>>>applications relied on it. > >>>>>> > >>>>>>To restore some compatibility, reset the codec if we receive a new > >>>>>>non-drain > >>>>>>packet using the old API after draining has completed. While this does > >>>>>>not give the same behaviour as the old API did, in the majority of > >>>>>>cases > >>>>>>it works and it does not require changes to any other part of the > >>>>>>decoding > >>>>>>code. > >>>>>> > >>>>>>Fixes ticket #6775 > >>>>>>Signed-off-by: James Cowgill <jcowgill@debian.org> > >>>>>>--- > >>>>>> libavcodec/decode.c | 5 +++++ > >>>>>> 1 file changed, 5 insertions(+) > >>>>>> > >>>>>>diff --git a/libavcodec/decode.c b/libavcodec/decode.c > >>>>>>index 86fe5aef52..2f1932fa85 100644 > >>>>>>--- a/libavcodec/decode.c > >>>>>>+++ b/libavcodec/decode.c > >>>>>>@@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, > >>>>>>AVFrame *frame, > >>>>>> > >>>>>> av_assert0(avci->compat_decode_consumed == 0); > >>>>>> > >>>>>>+ if (avci->draining_done && pkt && pkt->size != 0) { > >>>>>>+ av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after > >>>>>>EOF\n"); > >>>>>>+ avcodec_flush_buffers(avctx); > >>>>>>+ } > >>>>>>+ > >>>>> > >>>>>I don't think this is a good idea. Draining and not flushing > >>>>>afterwards is a bug in the calling code, and even before recent > >>>>>changes it would result in inconsistent behavior and even crashes > >>>>>(with select decoders). > >>>> > >>>>I am fully aware that this will only trigger if the calling code is > >>>>buggy. I am trying to avoid silent breakage of those applications doing > >>>>this when upgrading to ffmpeg 3.4. > >>>> > >>>>I was looking at the documentation of avcodec_decode_* recently because > >>>>of this and I had some trouble deciding if using the API this way was > >>>>incorrect. I expect the downstreams affected thought that what they were > >>>>doing was fine and then got angry when ffmpeg suddenly "broke" their > >>>>code. This patch at least allows some sort of "transitional period" > >>>>until downstreams update. > >>> > >>>I think the intent was to flush the codec by passing the NULL packets to > >>>it, so it makes a lot of sense to actually do that. Especially since by > >>>implicitly doing a flush, we can avoid the undefined behaviour/crashes > >>>on the codec side. > >>> > >>>Also this is only compatibility code, which probably will be removed at > >>>the next bump, I see no harm in making it as compatible as realistically > >>>possible. > >> > >>The old decode API is not scheduled for removal right now probably > >>because 99% of decoders need to be ported. > >>This compat code was written so the old API becomes a wrapper for the > >>new rather than the other way around, as it was up to 3.3. Supposedly a > >>good portion of the versatility of the new API would be handicapped > >>otherwise. > >> > > > >>Personally, I think this should be left as is. It is a good incentive > >>for downstream to migrate to the new API, as they technically were > >>misusing the old API to begin with. > > > >providing compatibility support for an old API that does not actually > >work with how applications used the old API is something a tad bit > >bizzare > > > >We want to minimize the work everyone has to do. > >The more time people have, the more they can spend on improving free > >software. > > > >If the old API is going to be removed, any work people have to do > >to hunt and track implementation changes in our old API the more > >they have wasted time. > >If you want people to spend their time on the new API, then you > >should not introduce issues in the old API that they need to > >workaround > >They that way just lost time (debug, fix, test) they could have spend > >on the new API or on anything else > > > > Applied and backported. Thanks alot ! [...]
diff --git a/libavcodec/decode.c b/libavcodec/decode.c index 86fe5aef52..2f1932fa85 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -726,6 +726,11 @@ static int compat_decode(AVCodecContext *avctx, AVFrame *frame, av_assert0(avci->compat_decode_consumed == 0); + if (avci->draining_done && pkt && pkt->size != 0) { + av_log(avctx, AV_LOG_WARNING, "Got unexpected packet after EOF\n"); + avcodec_flush_buffers(avctx); + } + *got_frame = 0; avci->compat_decode = 1;
In commit 061a0c14bb57 ("decode: restructure the core decoding code"), the deprecated avcodec_decode_* APIs were reworked so that they called into the new avcodec_send_packet / avcodec_receive_frame API. This had the side effect of prohibiting sending new packets containing data after a drain packet, but in previous versions of FFmpeg this "worked" and some applications relied on it. To restore some compatibility, reset the codec if we receive a new non-drain packet using the old API after draining has completed. While this does not give the same behaviour as the old API did, in the majority of cases it works and it does not require changes to any other part of the decoding code. Fixes ticket #6775 Signed-off-by: James Cowgill <jcowgill@debian.org> --- libavcodec/decode.c | 5 +++++ 1 file changed, 5 insertions(+)