[FFmpeg-devel] [PATCH] lavc: reset codec on receiving packet after EOF in compat_decode

[Michael Niedermayer]

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 at 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 at 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 !

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is what and why we do it that matters, not just one of them.
-------------- 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/20171209/585a12dc/attachment.sig>