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.
