[FFmpeg-devel] [PATCH v2] avcodec: add a get_encoder_buffer() callback to AVCodecContext
James Almer
jamrial at gmail.com
Fri Mar 12 21:59:16 EET 2021
On 3/12/2021 4:46 PM, James Almer wrote:
> On 3/12/2021 4:30 PM, Michael Niedermayer wrote:
>> On Fri, Mar 12, 2021 at 02:03:52PM -0300, James Almer wrote:
>>> On 3/12/2021 1:32 PM, Michael Niedermayer wrote:
>>>> 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);
>>>
>>> I ask again, where are you going with this? The alignment for
>>> AVPacket data
>>> buffers is defined: There is *none*.
>>
>> I simply stated that 'alignment would be "nice to have"'.
>> and then showed some cases where it would be usefull.
>
> But don't those cases already happen, and without required or guaranteed
> alignment?
>
>>
>> I guess where iam going with this is, is the API you add extensible?
>> That is if something is not supported now, can it be added later without
>> adding a new API.
>
> I should, it shares a signature with get_buffer2(). That means the
> packet to fill (Which fields can be read from it and set can be easily
> redefined), avctx so the user can have access to avctx->opaque and so we
> can eventually use something like a buffer pool in the default allocator
> callback, and a flags parameter to tell the callback there are
> requirements.
>
> Which makes me realize, maybe a flag to tell the callback "Alignment is
> required" could solve your concerns?
Actually, thinking about it, it's the same situation as always requiring
it. The mere existence of such a flag would require users of the old API
moving onto the new to redefine their buffers, since now they *may* need
to align them, when before they didn't. So not really an option.
>
>>
>> Thanks
>>
>> [...]
>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>
More information about the ffmpeg-devel
mailing list