[FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext

[James Almer]

On 2/20/2021 9:20 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/20/2021 8:41 PM, Lynne wrote:
>>> Feb 20, 2021, 21:53 by jamrial at gmail.com:
>>>
>>>> This callback is functionally the same as get_buffer2() is for
>>>> decoders, and
>>>> implements for the new encode API the functionality of the old encode
>>>> API had
>>>> where the user could provide their own buffers.
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> As suggested by Anton, here's get_buffer() for encoders. This way,
>>>> users of the
>>>> new encode API can still provide their own buffers, like they could
>>>> with the
>>>> old API, now that it's being removed.
>>>>
>>>> The initial implementation of the default callback uses the existing
>>>> method of
>>>> simply calling av_new_packet() for it. In the future, a buffer pool
>>>> could be
>>>> used instead.
>>>> Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that
>>>> keeps a
>>>> reference to a previous packet around?
>>>>
>>>
>>> Could be useful, and keeps it symmetrical with decoding, so I'd keep it,
>>> just in case.
>>>
>>>
>>>> Alternative names for the callback field and public default callback
>>>> function
>>>> are welcome, hence it being RFC.
>>>>
>>>
>>> get_enc_buffer() -> get_encoder_buffer()
>>> avcodec_default_get_enc_buffer-> avcodec_default_get_encoder_buffer()
>>> That's all I'd suggest. It's just 4 more characters.
>>
>> If others prefer that, I'll use it.
>>
>>>   
>>>> +    /**
>>>> +     * This callback is called at the beginning of each packet to
>>>> get a data
>>>> +     * buffer for it.
>>>> +     *
>>>> +     * The following field will be set in the packet before this
>>>> callback is
>>>> +     * called:
>>>> +     * - size (may by zero)
>>>> +     * This callback must use the above value to calculate the
>>>> required buffer size,
>>>> +     * which must padded by at least AV_INPUT_BUFFER_PADDING_SIZE
>>>> bytes.
>>>> +     *
>>>> +     * This callback must fill the following fields in the packet:
>>>> +     * - data
>>>> +     * - buf must contain a pointer to an AVBufferRef structure. The
>>>> packet's
>>>> +     *   data pointer must be contained in it.
>>>> +     *   See: av_buffer_create(), av_buffer_alloc(), and
>>>> av_buffer_ref().
>>>>
>>>
>>> Does a size of zero mean you can have a NULL packet data and buffer
>>> or a AV_INPUT_BUFFER_PADDING_SIZE-sized buffer?
>>
>> As i mentioned on IRC, calling ff_alloc_packet2() or av_new_packet()
>> with size 0 is valid (it will indeed allocate an AVBufferRef of padding
>> size bytes, with pkt->data pointing to it and pkt->size set to 0), so a
>> function that replaces the former and essentially wraps the latter
>> should probably also allow it.
>>
>> I didn't want to keep ambiguous cases in the doxy, but i can remove that
>> part. av_new_packet() doesn't mention it either after all.
>>
>>>
>>>
>>>> +     *
>>>> +     * If AV_CODEC_CAP_DR1 is not set then get_enc_buffer() must call
>>>> +     * avcodec_default_get_enc_buffer() instead of providing a
>>>> buffer allocated by
>>>> +     * some other means.
>>>> +     *
>>>> +     * If AV_GET_ENC_BUFFER_FLAG_REF is set in flags then the packet
>>>> may be reused
>>>> +     * (read and/or written to if it is writable) later by libavcodec.
>>>> +     *
>>>> +     * @see avcodec_default_get_enc_buffer()
>>>> +     *
>>>> +     * - encoding: Set by libavcodec, user can override.
>>>> +     * - decoding: unused
>>>> +     */
>>>> +    int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt,
>>>> int flags);
>>>>
>>>
>>> Maybe mention thread safety? Since in a frame-threaded encoder
>>> this may be called from different threads.
>>
>> So copy paste the paragraph "If frame multithreading is used, this
>> callback may be called from a different thread, but not from more than
>> one at once. Does not need to be reentrant" from get_buffer2()?.
>>
> I don't like the "not from more than one at once" part as this implies
> that we would have to somehow guard ff_alloc_packet2 by a mutex, which
> is currently not so.

ff_alloc_packet2() is not going to use this API as it uses this 
intermediary internal byte buffer that exists mainly for mpegvideoenc, 
so i decided to keep it independent, and any encoder adapted to use this 
callback will instead use the new function introduced in this patch.

I'll in any case use Lynne's suggestion.