| Message ID | 20210220205327.11515-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,RFC] avcodec: add a get_enc_buffer() callback to AVCodecContext | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
Feb 20, 2021, 21:53 by jamrial@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 <jamrial@gmail.com> > --- > 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.
On 2/20/2021 8:41 PM, Lynne wrote: > Feb 20, 2021, 21:53 by jamrial@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 <jamrial@gmail.com> >> --- >> 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-request@ffmpeg.org with subject "unsubscribe". >
James Almer: > On 2/20/2021 8:41 PM, Lynne wrote: >> Feb 20, 2021, 21:53 by jamrial@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 <jamrial@gmail.com> >>> --- >>> 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
Feb 21, 2021, 01:10 by jamrial@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.".
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 jamrial@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 <jamrial@gmail.com> >>>> --- >>>> 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.
Lynne: > Feb 21, 2021, 01:10 by jamrial@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
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(EINVAL); + } + + ret = av_new_packet(avpkt, avpkt->size); + if (ret < 0) + av_log(avctx, AV_LOG_ERROR, "Failed to allocate packet of size %d\n", avpkt->size); + + return ret; +} + +int ff_get_enc_buffer(AVCodecContext *avctx, AVPacket *avpkt, int64_t size, int flags) +{ + int ret; + + if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) + return AVERROR(EINVAL); + + av_assert0(!avpkt->data && !avpkt->buf); + + avpkt->size = size; + ret = avctx->get_enc_buffer(avctx, avpkt, flags); + if (ret < 0) + goto fail; + + if (!avpkt->data || !avpkt->buf) { + av_log(avctx, AV_LOG_ERROR, "No buffer returned by get_enc_buffer()\n"); + ret = AVERROR(EINVAL); + goto fail; + } + + ret = 0; +fail: + if (ret < 0) { + av_log(avctx, AV_LOG_ERROR, "get_enc_buffer() failed\n"); + av_packet_unref(avpkt); + } + + return ret; +} + /** * Pad last frame with silence. */ @@ -169,7 +215,7 @@ static int encode_simple_internal(AVCodecContext *avctx, AVPacket *avpkt) emms_c(); if (!ret && got_packet) { - if (avpkt->data) { + if (avpkt->data && !(avctx->codec->capabilities & AV_CODEC_CAP_DR1)) { ret = av_packet_make_refcounted(avpkt); if (ret < 0) goto end; @@ -377,6 +423,12 @@ static int compat_encode(AVCodecContext *avctx, AVPacket *avpkt, av_log(avctx, AV_LOG_WARNING, "AVFrame.width or height is not set\n"); } + if (avctx->codec->capabilities & AV_CODEC_CAP_DR1) { + av_log(avctx, AV_LOG_WARNING, "The deprecated avcodec_encode_* API does not support " + "AV_CODEC_CAP_DR1 encoders\n"); + return AVERROR(ENOSYS); + } + ret = avcodec_send_frame(avctx, frame); if (ret == AVERROR_EOF) ret = 0; diff --git a/libavcodec/encode.h b/libavcodec/encode.h index dfa9cb2d97..93fc30db43 100644 --- a/libavcodec/encode.h +++ b/libavcodec/encode.h @@ -36,4 +36,11 @@ */ int ff_encode_get_frame(AVCodecContext *avctx, AVFrame *frame); +/** + * Get a buffer for a packet. This is a wrapper around + * AVCodecContext.get_enc_buffer() and should be used instead calling get_encbuffer() + * directly. + */ +int ff_get_enc_buffer(AVCodecContext *avctx, AVPacket *avpkt, int64_t size, int flags); + #endif /* AVCODEC_ENCODE_H */ diff --git a/libavcodec/options.c b/libavcodec/options.c index 4bbf74ec7f..c32d603d0c 100644 --- a/libavcodec/options.c +++ b/libavcodec/options.c @@ -129,6 +129,7 @@ static int init_context_defaults(AVCodecContext *s, const AVCodec *codec) s->framerate = (AVRational){ 0, 1 }; s->pkt_timebase = (AVRational){ 0, 1 }; s->get_buffer2 = avcodec_default_get_buffer2; + s->get_enc_buffer = avcodec_default_get_enc_buffer; s->get_format = avcodec_default_get_format; s->execute = avcodec_default_execute; s->execute2 = avcodec_default_execute2;
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 <jamrial@gmail.com> --- 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(-)