On Fri, Mar 12, 2021 at 04:59:16PM -0300, James Almer wrote:
> 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 <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.
> > > > > > > > > > >
> > > > > > > > > > > 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.yes, maybe all this is overengeneering it. so as said on irc, iam ok with the patch with a minor clarification on the flags being hints Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament.
signature.asc
Description: PGP signature
_______________________________________________ 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".
