[FFmpeg-devel] [PATCH v2] avcodec: add a get_encoder_buffer() callback to AVCodecContext

Fri Mar 12 22:53:33 EET 2021

Andreas Rheinhardt:
> Michael Niedermayer:
>> On Thu, Mar 11, 2021 at 02:18:36PM -0300, James Almer wrote:
>>> On 3/11/2021 1:35 PM, Michael Niedermayer wrote:
>>>> On Wed, Mar 10, 2021 at 05:59:11PM -0300, James Almer wrote:
>>>>> On 3/10/2021 5:18 PM, Michael Niedermayer wrote:
>>>>>> On Mon, Feb 22, 2021 at 07:27:34PM -0300, James Almer wrote:
>>>>>>> On 2/21/2021 6:04 PM, James Almer wrote:
>>>>>>>> On 2/21/2021 5:29 PM, Mark Thompson wrote:
>>>>>>>>> On 21/02/2021 20:00, James Almer wrote:
>>>>>>>>>> On 2/21/2021 4:13 PM, Mark Thompson wrote:
>>>>>>>>>>> On 21/02/2021 17:35, James Almer wrote:
>>>>>>>>>>>> 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>
>>>>>>>>>>>> ---
>>>>>>>>>>>> Used the names Lynne suggested this time, plus a line
>>>>>>>>>>>> about how the callback
>>>>>>>>>>>> must be thread safe.
>>>>>>>>>>>>
>>>>>>>>>>>>     libavcodec/avcodec.h | 45 ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>     libavcodec/codec.h   |  8 ++++---
>>>>>>>>>>>>     libavcodec/encode.c  | 54
>>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++-
>>>>>>>>>>>>     libavcodec/encode.h  |  8 +++++++
>>>>>>>>>>>>     libavcodec/options.c |  1 +
>>>>>>>>>>>>     5 files changed, 112 insertions(+), 4 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
>>>>>>>>>>>> index 7dbf083a24..e60eb16ce1 100644
>>>>>>>>>>>> --- a/libavcodec/avcodec.h
>>>>>>>>>>>> +++ b/libavcodec/avcodec.h
>>>>>>>>>>>> @@ -513,6 +513,11 @@ typedef struct AVProducerReferenceTime {
>>>>>>>>>>>>      */
>>>>>>>>>>>>     #define AV_GET_BUFFER_FLAG_REF (1 << 0)
>>>>>>>>>>>> +/**
>>>>>>>>>>>> + * The encoder will keep a reference to the packet and
>>>>>>>>>>>> may reuse it later.
>>>>>>>>>>>> + */
>>>>>>>>>>>> +#define AV_GET_ENCODER_BUFFER_FLAG_REF (1 << 0)
>>>>>>>>>>>> +
>>>>>>>>>>>>     struct AVCodecInternal;
>>>>>>>>>>>>     /**
>>>>>>>>>>>> @@ -2346,6 +2351,39 @@ typedef struct AVCodecContext {
>>>>>>>>>>>>          * - encoding: set by user
>>>>>>>>>>>>          */
>>>>>>>>>>>>         int export_side_data;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /**
>>>>>>>>>>>> +     * 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
>>>>>>>>>>>> +     * 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
>>>>>>>>>>>
>>>>>>>>>>> Is the data pointer allowed to be in write-only memory?
>>>>>>>>>>
>>>>>>>>>> I'm not sure what the use case for this would be, so probably no?
>>>>>>>>>
>>>>>>>>> The two use-cases I see for this API are:
>>>>>>>>>
>>>>>>>>> * You want to avoid a copy when combining the output with something
>>>>>>>>> else.  E.g. you pass a pointer to the block of memory following
>>>>>>>>> where you are going to put your header data (for something you are
>>>>>>>>> going to send over the network, say).
>>>>>>>>>
>>>>>>>>> * You want to avoid a copy when passing the output directly to
>>>>>>>>> something external.  E.g. you pass a pointer to a memory-mapped
>>>>>>>>> device buffer (such as a V4L2 buffer, say).
>>>>>>>>>
>>>>>>>>> In the second case, write-only memory on an external device seems
>>>>>>>>> possible, as does memory which is, say, readable but uncached, so
>>>>>>>>> reading it is a really bad idea.
>>>>>>>>
>>>>>>>> Allowing the second case would depend on how encoders behave. Some may
>>>>>>>> attempt to read data already written to the output packet. It's not like
>>>>>>>> all of them allocate the packet, do a memcpy from an internal buffer,
>>>>>>>> then return.
>>>>>>>> There is also the flag meant to signal that the encoder will keep a
>>>>>>>> reference to the packet around, which more or less implies it will be
>>>>>>>> read later in the encoding process.
>>>>>>>>
>>>>>>>> The doxy for avcodec_encode_video2(), which allowed the user to provide
>>>>>>>> their own buffers in the output packet, does not mention any kind of
>>>>>>>> requirement for the data pointer, so I don't think we can say it's an
>>>>>>>> allowed scenario here either.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> Does it have any alignment requirements?
>>>>>>>>>>
>>>>>>>>>> No, just padding. AVPacket doesn't require alignment for the payload.
>>>>>>>>>
>>>>>>>>> I think say that explicitly.  avcodec_default_get_encoder_buffer()
>>>>>>>>> does give you aligned memory, even though it isn't needed.
>>>>>>>>
>>>>>>>> Would saying "There's no alignment requirement for the data pointer" add
>>>>>>>> anything of value to the doxy? If i don't mention any kind of alignment
>>>>>>>> requirement, it's because there isn't any, and it's implicit.
>>>>>>>> I listed the requirements the user needs to keep in mind, like the
>>>>>>>> padding and the need for an AVBufferRef. But if you think it's worth
>>>>>>>> adding, then sure.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>> +     * - 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().
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * If AV_CODEC_CAP_DR1 is not set then
>>>>>>>>>>>> get_encoder_buffer() must call
>>>>>>>>>>>> +     * avcodec_default_get_encoder_buffer() instead of
>>>>>>>>>>>> providing a buffer allocated by
>>>>>>>>>>>> +     * some other means.
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * If AV_GET_ENCODER_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.
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * This callback must be thread-safe, as when frame
>>>>>>>>>>>> multithreading is used, it may
>>>>>>>>>>>> +     * be called from multiple threads simultaneously.
>>>>>>>>>>>
>>>>>>>>>>> Allowing simulatenous calls feels unexpectedly tricky.  Is
>>>>>>>>>>> it really necessary?
>>>>>>>>>>
>>>>>>>>>> This was a suggestion by Lynne, i personally don't know. We
>>>>>>>>>> support frame threading encoding (For intra-only codecs), but
>>>>>>>>>> currently ff_alloc_packet2() does not seem to be thread safe,
>>>>>>>>>> seeing it calls av_fast_padded_malloc(), yet it's called by
>>>>>>>>>> frame threaded encoders.
>>>>>>>>>> Should i remove this?
>>>>>>>>>
>>>>>>>>> I don't know, I was asking only because it sounds tricky.  For cases
>>>>>>>>> with a limited number of buffers available (like memory-mapped
>>>>>>>>> devices) you are going to need locking anyway, so maybe rentrancy
>>>>>>>>> adds no additional inconvenience.
>>>>>>>>>
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * @see avcodec_default_get_encoder_buffer()
>>>>>>>>>>>> +     *
>>>>>>>>>>>> +     * - encoding: Set by libavcodec, user can override.
>>>>>>>>>>>> +     * - decoding: unused
>>>>>>>>>>>> +     */
>>>>>>>>>>>> +    int (*get_encoder_buffer)(struct AVCodecContext *s,
>>>>>>>>>>>> AVPacket *pkt, int flags);
>>>>>>>>>>>
>>>>>>>>>>> Can the encoder ask for arbitrarily many packets?
>>>>>>>>>>>
>>>>>>>>>>> Can the user return "not yet" somehow to this if they have a
>>>>>>>>>>> fixed output buffer pool but no buffer is currently
>>>>>>>>>>> available?
>>>>>>>>>>
>>>>>>>>>> No, as is it can't. Return values < 0 are considered errors.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I don't much like the idea of the user suspending the thread
>>>>>>>>>>> in the callback until they have some available, which might
>>>>>>>>>>> work in some cases but might also deadlock if an
>>>>>>>>>>> avcodec_receive_packet() call is blocked by it.
>>>>>>>>>>
>>>>>>>>>> Can we make what's in essence a malloc() call return something
>>>>>>>>>> like EAGAIN, and this in turn be propagated back to
>>>>>>>>>> encode_receive_packet_internal()?
>>>>>>>>>
>>>>>>>>> Maybe, or if it has many threads maybe it could wait for something
>>>>>>>>> else to finish first.
>>>>>>>>>
>>>>>>>>>> Couldn't this potentially end up in the forbidden scenario of
>>>>>>>>>> avcodec_send_frame() and avcodec_receive_packet() both returning
>>>>>>>>>> EAGAIN?
>>>>>>>>>
>>>>>>>>> Yes.  If the forbidden case happens then the encoder is stuck anyway
>>>>>>>>> and can't make any forward progress so we need to error out
>>>>>>>>> properly, but the EAGAIN return isn't needed if there is something
>>>>>>>>> else to do on another thread.
>>>>>>>>
>>>>>>>> Ok, but I'm not familiar or knowledgeable enough with the frame thread
>>>>>>>> encoder code to implement this.
>>>>>>>
>>>>>>> Looked at bit into this. AVCodec->encode2() based encoders don't support
>>>>>>> returning EAGAIN at all, as it completely breaks the frame threading logic.
>>>>>>> It would require a considerable rewrite in order to re-add a task that
>>>>>>> didn't fail but also didn't succeed.
>>>>>>>
>>>>>>> Non frame threading encoders could probably support it with some minimal
>>>>>>> changes, but i don't think suddenly letting an scenario that was until now
>>>>>>> guaranteed to never happen start happening (avcodec_send_frame() and
>>>>>>> avcodec_receive_packet() both returning EAGAIN) is a good idea. It's an API
>>>>>>> break.
>>>>>>> Letting the user's custom get_encode_buffer() callback suspend the thread is
>>>>>>> IMO acceptable. In frame threading scenarios, the other threads are still
>>>>>>> working on their own packets (afaics none depends on the others, since it's
>>>>>>> intra only encoders only).
>>>>>>
>>>>>> I think it was not suggested in the thread so:
>>>>>> if the users allocation fails the code can fallback to the default allocator
>>>>>> That would lead to the relation:
>>>>>> If a users allocator can fail (out of buffers) it must be able to handle
>>>>>> that only some of the returned packets are from its own allocator
>>>>>
>>>>> In general, custom allocators are used when the caller doesn't want to use
>>>>> the default one. But yes, they could use
>>>>> avcodec_default_get_encoder_buffer() as fallback, which is why it was added
>>>>> to begin with. Same applies to get_buffer2() custom implementations, and so
>>>>> far i don't think anybody had issues identifying what allocated a packet
>>>>> buffer.
>>>>>
>>>>> One of the additions to AVPacket people were talking about was a user opaque
>>>>> field that libav* would never touch or look at beyond propagating them
>>>>> around all the way to the output AVFrame, if any. This opaque field could
>>>>> perhaps store such allocator specific information the caller could use to
>>>>> identify packets allocated by their own allocator, or those by
>>>>> avcodec_default_get_encoder_buffer().
>>>>>
>>>>>>
>>>>>> About alignment, we should at least recommand that allocated packets are
>>>>>> aligned not less than what out av_malloc() would align to.
>>>>>> Is there a reason to align less ?
>>>>>
>>>>> There's no alignment requirement for AVPacket->data, and av_new_packet()
>>>>> uses av_buffer_realloc(), which does not guarantee any alignment whatsoever
>>>>> on platforms other than Windows. So basically, packet payload buffers
>>>>> allocated by our own helpers never had any alignment.
>>>>
>>>> for the purpose of exporting raw images, alignment would be "nice to have"
>>>> because later filters may need it or need to memcpy
>>>
>>> Filters don't use AVPackets, they use AVFrames.
>>
>> demuxers return AVPackets, so do encoders.
>> These can contain raw frames.
>>
>> also i see for example in rawdec:
>> frame->buf[0] = av_buffer_ref(avpkt->buf);
>>
> 
> That seems to be completely broken: I see no check for whether the
> packet is writable at all. Will investigate.
> 
> - Andreas
> 
It is indeed possible for rawdec to write to non-writable buffers here.
It at least "works" with rgba64be. The checksums of a streamcopied track
change when one also decodes the track.

- Andreas