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

[Michael Niedermayer]

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 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.

i agree and i would appreciate if this gets resolved one way or another
so i can make the release

Thanks

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- 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/20171126/78574df8/attachment.sig>