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

[Marton Balint]

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.

Regards,
Marton