Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
On 2/20/2021 9:20 PM, Andreas Rheinhardt wrote: James Almer: On 2/20/2021 8:41 PM, Lynne wrote: Feb 20, 2021, 21:53 by jamr...@gmail.com: 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 --- As suggested by Anton, here's get_buffer() for encoders. This way, users of the new encode API can still provide their own buffers, like they could with the old API, now that it's being removed. The initial implementation of the default callback uses the existing method of simply calling av_new_packet() for it. In the future, a buffer pool could be used instead. Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that keeps a reference to a previous packet around? Could be useful, and keeps it symmetrical with decoding, so I'd keep it, just in case. Alternative names for the callback field and public default callback function are welcome, hence it being RFC. get_enc_buffer() -> get_encoder_buffer() avcodec_default_get_enc_buffer-> avcodec_default_get_encoder_buffer() That's all I'd suggest. It's just 4 more characters. If others prefer that, I'll use it. + /** + * 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 (may by zero) + * 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 + * - 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(). Does a size of zero mean you can have a NULL packet data and buffer or a AV_INPUT_BUFFER_PADDING_SIZE-sized buffer? As i mentioned on IRC, calling ff_alloc_packet2() or av_new_packet() with size 0 is valid (it will indeed allocate an AVBufferRef of padding size bytes, with pkt->data pointing to it and pkt->size set to 0), so a function that replaces the former and essentially wraps the latter should probably also allow it. I didn't want to keep ambiguous cases in the doxy, but i can remove that part. av_new_packet() doesn't mention it either after all. + * + * If AV_CODEC_CAP_DR1 is not set then get_enc_buffer() must call + * avcodec_default_get_enc_buffer() instead of providing a buffer allocated by + * some other means. + * + * If AV_GET_ENC_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. + * + * @see avcodec_default_get_enc_buffer() + * + * - encoding: Set by libavcodec, user can override. + * - decoding: unused + */ + int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt, int flags); Maybe mention thread safety? Since in a frame-threaded encoder this may be called from different threads. So copy paste the paragraph "If frame multithreading is used, this callback may be called from a different thread, but not from more than one at once. Does not need to be reentrant" from get_buffer2()?. I don't like the "not from more than one at once" part as this implies that we would have to somehow guard ff_alloc_packet2 by a mutex, which is currently not so. ff_alloc_packet2() is not going to use this API as it uses this intermediary internal byte buffer that exists mainly for mpegvideoenc, so i decided to keep it independent, and any encoder adapted to use this callback will instead use the new function introduced in this patch. I'll in any case use Lynne's suggestion. ___ 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".
Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
Lynne: > Feb 21, 2021, 01:10 by jamr...@gmail.com: > >> On 2/20/2021 8:41 PM, Lynne wrote: >> >>> Maybe mention thread safety? Since in a frame-threaded encoder >>> this may be called from different threads. >>> >> >> So copy paste the paragraph "If frame multithreading is used, this callback >> may be called from a different thread, but not from more than one at once. >> Does not need to be reentrant" from get_buffer2()?. >> > > No, that's the previous definition before we deprecated > "thread_safe_callbacks". > So something like "The callback should be thread-safe, as when frame > multithreading > is used, it may be called from multiple threads simultaneously.". If you look at thread_get_buffer_internal, you'll see that calls to ff_get_buffer never happen simultaneously from different threads. Even with thread_safe_callbacks. The deprecation hasn't changed that. - Andreas ___ 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".
Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
Feb 21, 2021, 01:10 by jamr...@gmail.com: > On 2/20/2021 8:41 PM, Lynne wrote: > >> Maybe mention thread safety? Since in a frame-threaded encoder >> this may be called from different threads. >> > > So copy paste the paragraph "If frame multithreading is used, this callback > may be called from a different thread, but not from more than one at once. > Does not need to be reentrant" from get_buffer2()?. > No, that's the previous definition before we deprecated "thread_safe_callbacks". So something like "The callback should be thread-safe, as when frame multithreading is used, it may be called from multiple threads simultaneously.". ___ 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".
Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
James Almer: > On 2/20/2021 8:41 PM, Lynne wrote: >> Feb 20, 2021, 21:53 by jamr...@gmail.com: >> >>> 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 >>> --- >>> As suggested by Anton, here's get_buffer() for encoders. This way, >>> users of the >>> new encode API can still provide their own buffers, like they could >>> with the >>> old API, now that it's being removed. >>> >>> The initial implementation of the default callback uses the existing >>> method of >>> simply calling av_new_packet() for it. In the future, a buffer pool >>> could be >>> used instead. >>> Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that >>> keeps a >>> reference to a previous packet around? >>> >> >> Could be useful, and keeps it symmetrical with decoding, so I'd keep it, >> just in case. >> >> >>> Alternative names for the callback field and public default callback >>> function >>> are welcome, hence it being RFC. >>> >> >> get_enc_buffer() -> get_encoder_buffer() >> avcodec_default_get_enc_buffer-> avcodec_default_get_encoder_buffer() >> That's all I'd suggest. It's just 4 more characters. > > If others prefer that, I'll use it. > >> >>> + /** >>> + * 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 (may by zero) >>> + * 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 >>> + * - 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(). >>> >> >> Does a size of zero mean you can have a NULL packet data and buffer >> or a AV_INPUT_BUFFER_PADDING_SIZE-sized buffer? > > As i mentioned on IRC, calling ff_alloc_packet2() or av_new_packet() > with size 0 is valid (it will indeed allocate an AVBufferRef of padding > size bytes, with pkt->data pointing to it and pkt->size set to 0), so a > function that replaces the former and essentially wraps the latter > should probably also allow it. > > I didn't want to keep ambiguous cases in the doxy, but i can remove that > part. av_new_packet() doesn't mention it either after all. > >> >> >>> + * >>> + * If AV_CODEC_CAP_DR1 is not set then get_enc_buffer() must call >>> + * avcodec_default_get_enc_buffer() instead of providing a >>> buffer allocated by >>> + * some other means. >>> + * >>> + * If AV_GET_ENC_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. >>> + * >>> + * @see avcodec_default_get_enc_buffer() >>> + * >>> + * - encoding: Set by libavcodec, user can override. >>> + * - decoding: unused >>> + */ >>> + int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt, >>> int flags); >>> >> >> Maybe mention thread safety? Since in a frame-threaded encoder >> this may be called from different threads. > > So copy paste the paragraph "If frame multithreading is used, this > callback may be called from a different thread, but not from more than > one at once. Does not need to be reentrant" from get_buffer2()?. > I don't like the "not from more than one at once" part as this implies that we would have to somehow guard ff_alloc_packet2 by a mutex, which is currently not so. - Andreas ___ 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".
Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
On 2/20/2021 8:41 PM, Lynne wrote: Feb 20, 2021, 21:53 by jamr...@gmail.com: 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 --- As suggested by Anton, here's get_buffer() for encoders. This way, users of the new encode API can still provide their own buffers, like they could with the old API, now that it's being removed. The initial implementation of the default callback uses the existing method of simply calling av_new_packet() for it. In the future, a buffer pool could be used instead. Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that keeps a reference to a previous packet around? Could be useful, and keeps it symmetrical with decoding, so I'd keep it, just in case. Alternative names for the callback field and public default callback function are welcome, hence it being RFC. get_enc_buffer() -> get_encoder_buffer() avcodec_default_get_enc_buffer-> avcodec_default_get_encoder_buffer() That's all I'd suggest. It's just 4 more characters. If others prefer that, I'll use it. +/** + * 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 (may by zero) + * 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 + * - 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(). Does a size of zero mean you can have a NULL packet data and buffer or a AV_INPUT_BUFFER_PADDING_SIZE-sized buffer? As i mentioned on IRC, calling ff_alloc_packet2() or av_new_packet() with size 0 is valid (it will indeed allocate an AVBufferRef of padding size bytes, with pkt->data pointing to it and pkt->size set to 0), so a function that replaces the former and essentially wraps the latter should probably also allow it. I didn't want to keep ambiguous cases in the doxy, but i can remove that part. av_new_packet() doesn't mention it either after all. + * + * If AV_CODEC_CAP_DR1 is not set then get_enc_buffer() must call + * avcodec_default_get_enc_buffer() instead of providing a buffer allocated by + * some other means. + * + * If AV_GET_ENC_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. + * + * @see avcodec_default_get_enc_buffer() + * + * - encoding: Set by libavcodec, user can override. + * - decoding: unused + */ +int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt, int flags); Maybe mention thread safety? Since in a frame-threaded encoder this may be called from different threads. So copy paste the paragraph "If frame multithreading is used, this callback may be called from a different thread, but not from more than one at once. Does not need to be reentrant" from get_buffer2()?. Rest looks good. ___ 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". ___ 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".
Re: [FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
Feb 20, 2021, 21:53 by jamr...@gmail.com: > 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 > --- > As suggested by Anton, here's get_buffer() for encoders. This way, users of > the > new encode API can still provide their own buffers, like they could with the > old API, now that it's being removed. > > The initial implementation of the default callback uses the existing method of > simply calling av_new_packet() for it. In the future, a buffer pool could be > used instead. > Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that keeps a > reference to a previous packet around? > Could be useful, and keeps it symmetrical with decoding, so I'd keep it, just in case. > Alternative names for the callback field and public default callback function > are welcome, hence it being RFC. > get_enc_buffer() -> get_encoder_buffer() avcodec_default_get_enc_buffer-> avcodec_default_get_encoder_buffer() That's all I'd suggest. It's just 4 more characters. > +/** > + * 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 (may by zero) > + * 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 > + * - 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(). > Does a size of zero mean you can have a NULL packet data and buffer or a AV_INPUT_BUFFER_PADDING_SIZE-sized buffer? > + * > + * If AV_CODEC_CAP_DR1 is not set then get_enc_buffer() must call > + * avcodec_default_get_enc_buffer() instead of providing a buffer > allocated by > + * some other means. > + * > + * If AV_GET_ENC_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. > + * > + * @see avcodec_default_get_enc_buffer() > + * > + * - encoding: Set by libavcodec, user can override. > + * - decoding: unused > + */ > +int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt, int > flags); > Maybe mention thread safety? Since in a frame-threaded encoder this may be called from different threads. Rest looks good. ___ 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".
[FFmpeg-devel] [RFC][PATCH] avcodec: add a get_enc_buffer() callback to AVCodecContext
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
---
As suggested by Anton, here's get_buffer() for encoders. This way, users of the
new encode API can still provide their own buffers, like they could with the
old API, now that it's being removed.
The initial implementation of the default callback uses the existing method of
simply calling av_new_packet() for it. In the future, a buffer pool could be
used instead.
Is AV_GET_ENC_BUFFER_FLAG_REF useful here? Is there an encoder that keeps a
reference to a previous packet around?
Alternative names for the callback field and public default callback function
are welcome, hence it being RFC.
libavcodec/avcodec.h | 42 ++
libavcodec/codec.h | 8 ---
libavcodec/encode.c | 54 +++-
libavcodec/encode.h | 7 ++
libavcodec/options.c | 1 +
5 files changed, 108 insertions(+), 4 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 7dbf083a24..5b4a731e9f 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_ENC_BUFFER_FLAG_REF (1 << 0)
+
struct AVCodecInternal;
/**
@@ -2346,6 +2351,36 @@ 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 (may by zero)
+ * 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
+ * - 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_enc_buffer() must call
+ * avcodec_default_get_enc_buffer() instead of providing a buffer
allocated by
+ * some other means.
+ *
+ * If AV_GET_ENC_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.
+ *
+ * @see avcodec_default_get_enc_buffer()
+ *
+ * - encoding: Set by libavcodec, user can override.
+ * - decoding: unused
+ */
+int (*get_enc_buffer)(struct AVCodecContext *s, AVPacket *pkt, int flags);
} AVCodecContext;
#if FF_API_CODEC_GET_SET
@@ -2920,6 +2955,13 @@ void avsubtitle_free(AVSubtitle *sub);
*/
int avcodec_default_get_buffer2(AVCodecContext *s, AVFrame *frame, int flags);
+/**
+ * The default callback for AVCodecContext.get_enc_buffer(). It is made public
so
+ * it can be called by custom get_enc_buffer() implementations for encoders
without
+ * AV_CODEC_CAP_DR1 set.
+ */
+int avcodec_default_get_enc_buffer(AVCodecContext *s, AVPacket *pkt, int
flags);
+
/**
* Modify width and height values so that they will result in a memory
* buffer that is acceptable for the codec if you do not use any horizontal
diff --git a/libavcodec/codec.h b/libavcodec/codec.h
index 0ccbf0eb19..c3460e82ac 100644
--- a/libavcodec/codec.h
+++ b/libavcodec/codec.h
@@ -43,9 +43,11 @@
*/
#define AV_CODEC_CAP_DRAW_HORIZ_BAND (1 << 0)
/**
- * Codec uses get_buffer() for allocating buffers and supports custom
allocators.
- * If not set, it might not use get_buffer() at all or use operations that
- * assume the buffer was allocated by avcodec_default_get_buffer.
+ * Codec uses get_buffer() or get_enc_buffer() for allocating buffers and
+ * supports custom allocators.
+ * If not set, it might not use get_buffer() or get_enc_buffer() at all, or
+ * use operations that assume the buffer was allocated by
+ * avcodec_default_get_buffer2 or avcodec_default_get_enc_buffer.
*/
#define AV_CODEC_CAP_DR1 (1 << 1)
#define AV_CODEC_CAP_TRUNCATED (1 << 3)
diff --git a/libavcodec/encode.c b/libavcodec/encode.c
index 282337e453..f464ad66b2 100644
--- a/libavcodec/encode.c
+++ b/libavcodec/encode.c
@@ -56,6 +56,52 @@ int ff_alloc_packet2(AVCodecContext *avctx, AVPacket *avpkt,
int64_t size, int64
return 0;
}
+int avcodec_default_get_enc_buffer(AVCodecContext *avctx, AVPacket *avpkt, int
flags)
+{
+int ret;
+
+if (avpkt->data || avpkt->buf) {
+av_log(avctx, AV_LOG_ERROR, "avpkt->{data,buf} != NULL in
avcodec_default_get_enc_buffer()\n");
+return AVERROR(EINVA
