[FFmpeg-devel] [PATCH] avcodec/nvenc: adapt to the new internal encode API

[James Almer]

On 4/9/2020 1:45 PM, Philip Langdale wrote:
> On Thu, 9 Apr 2020 11:20:09 -0300
> James Almer <jamrial at gmail.com> wrote:
> 
>> On 4/8/2020 7:04 PM, Philip Langdale wrote:
>>> On Wed,  8 Apr 2020 14:58:36 -0300
>>> James Almer <jamrial at gmail.com> wrote:
>>>   
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> This removes the encode2() implementation as it'll never be used
>>>> if a receive_packet() one exists, and the flush() implementation
>>>> since according to Anton Khirnov avcodec_flush_buffers() is not
>>>> meant to be used with encoders, where its behavior is undefined.  
>>>
>>> Nevertheless, there is a use case for flushing an encoder (see
>>> 3ea705), and when you call avcodec_flush_buffers() in nvenc, it
>>> does the right thing.
>>>
>>> Removing this will return us to the world before that change where
>>> there was no way to flush the encoder, and the original bug reporter
>>> was asking about adding an API to do it.
>>>
>>> It seems the right thing to do is to define the behaviour - which
>>> seems reasonable as-is today and documment that encoders can
>>> implement flush if necessary. Or we'll just end up adding a new API
>>> that flushes encoders but has a different name...
>>>
>>> --phil  
>>
>> I'm not against it, but as Marton said the function as it is is not
>> enough. The changes Anton asked me to remove would need to be re-added
>> (trivial to do), and the threading related code would probably need
>> some changes, if anything to not call decoding specific functions on
>> an encoder context.
>>
>> I like the codec capabilities suggestion from Marton. Could you look
>> into it? I'll change this patch to not remove the flush call while at
>> it.
> 
> Yes, please leave the flush as-is for now so we don't regress. At the
> very least I think adding an encoder vs decoder path into the function
> makes sense and we can skip all the decoder specific parts. Today, the
> emergent behaviour is that an encoder is only affected by the flush
> call if it provides a flush callback - none of the other parts will
> alter its state

No, right now buffer_pkt is unrefferenced, and buffer_pkt_valid and
draining are reset to 0 by this function. All of them fields used by the
encode API, before and after this patchset.
If an encoder doesn't have a flush() callback, then the encoder state
will be altered and things could behave in an unpredictable way.

, so perhaps the simple thing to do is have the encoder
> path just call the callback if it's there and naturally do nothing if
> not. That removes the need for a separate capability flag.

Yes, but how will the user know what encoder lets them flush without the
need to recreate the context, and which doesn't? avcodec_flush_buffers()
could do something or it could not, and the user will never know because
it doesn't return any kind of error code or result.

avcodec_flush_buffers() can easily be turned into a no-op for encoders
that don't implement a flush callback, but a codec cap or a similar
solution would let user know when he has no other option than to
recreate the context.