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

[Marton Balint]

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.

Regards,
Marton