On 22/02/2021 22:27, 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.
Fair enough. If the tricky cases aren't allowed then there is no problem :)
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).
Ok, so the only consistent behaviour is that the callback failing is fatal
(just like it is with get_buffer2). That's fine, then.
No more from me, looks good.
Thanks,
- Mark
_______________________________________________
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".
