Feb 21, 2021, 22:04 by jamr...@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 <jamr...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
