[FFmpeg-devel] [PATCH] ffmpeg; check return code of avcodec_send_frame when flushing encoders

[Marton Balint]

On Tue, 18 Apr 2017, Michael Niedermayer wrote:

> On Tue, Apr 18, 2017 at 08:46:24PM +0200, Marton Balint wrote:
>>
>>
>> On Tue, 18 Apr 2017, Michael Niedermayer wrote:
>>
>>> On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote:
>>>> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit :
>>>>>> +                while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) {
>>>>>> +                    ret = avcodec_send_frame(enc, NULL);
>>>>
>>>> The doc says:
>>>>
>>>> # The functions will not return AVERROR(EAGAIN), unless you forgot to
>>>> # enter draining mode.
>>>
>>> The full paragraph in the docs which you qoted from says this:
>>> * - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet()
>>> *   (encoding) in a loop until AVERROR_EOF is returned. The functions will
>>> *   not return AVERROR(EAGAIN), unless you forgot to enter draining mode.
>>>
>>> the patch adds a check to avcodec_send_frame()
>>>
>>>
>>>>
>>>>> can the code be changed to not require this ?
>>>>
>>>> I would say the code does not require this as is.
>>>
>>> For decoding theres an explicit
>>> "Sending the first flush packet will return success."
>>
>> As far as I see this sentence is only true if there was no decoding
>> error in the legacy API during a flush. So this should probably be
>> changed to something like "The first flush packet will not return
>> AVERROR_EOF, if it returns success then any subsequent flush packets
>> will return AVERROR_EOF." By the way we also guarantee this at
>> libavcodec level, so it is harder to write a codec with the new API
>> which violates this.
>>
>>> I cannot find similar for encoding, which is the case the patch changes
>>> and what i think should be fixed if possible as it would be simpler,
>>> making the patch unneeded.
>>> Its quite possible iam missing something that makes it uneeded though
>>
>> The same is true for send_frame, based on the code involving the
>> legacy API.
>>
>> We can of course decide to change the code involving the legacy API
>> and enforce that flushing always succeed, but I'd rather keep it as
>> is, even if that means a bit more error checking. It would be ugly
>> API-wise that you sometimes have to check the return value of a
>> function, sometimes you don't.
>
> iam happy with any solution or the patch as is
>

Unless there are no further comments I will apply this as is and backport 
it to 3.3 as well.

Regards,
Marton