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

[Andreas Rheinhardt]

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.

- Andreas