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

[Lynne]

Feb 21, 2021, 22:04 by jamrial at gmail.com:

> 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.
>

+1. That was one reason I wanted the flag kept.


>>>> 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.
>

I definitely think packet buffer alignment should be mandated. v210 and various
packing/unpacking codecs' SIMD depend on this.
Should have the same rules as the alignment on AVFrames.


>>>>> +     * - 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.
>>

Are there any actual devices or APIs which put a limit on the number of packets a user
can have at one point? I'm not aware of any.
I think users should handle locking themselves if they need to.