[FFmpeg-devel] [PATCH v2] avcodec: add a get_encoder_buffer() callback to AVCodecContext
James Almer
jamrial at gmail.com
Sun Feb 21 22:00:01 EET 2021
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 <jamrial at 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?
> Does it have any alignment requirements?
No, just padding. AVPacket doesn't require alignment for the payload.
>
>> + * - 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?
>
>> + *
>> + * @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()? Couldn't this potentially end up in
the forbidden scenario of avcodec_send_frame() and
avcodec_receive_packet() both returning EAGAIN?
>
> (For get_buffer2 we have a bit of consideration of this problem for
> hardware frames which might have limited numbers via
> avcodec_get_hw_frames_parameters(), though the general case does not
> have a solution.)
>
>> } AVCodecContext;
>> #if FF_API_CODEC_GET_SET
>> @@ -2920,6 +2958,13 @@ void avsubtitle_free(AVSubtitle *sub);
>> */
>> int avcodec_default_get_buffer2(AVCodecContext *s, AVFrame *frame,
>> int flags);
>> +/**
>> + * The default callback for AVCodecContext.get_encoder_buffer(). It
>> is made public so
>> + * it can be called by custom get_encoder_buffer() implementations
>> for encoders without
>> + * AV_CODEC_CAP_DR1 set.
>> + */
>> +int avcodec_default_get_encoder_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..a679fdc9e1 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_encoder_buffer() for allocating
>> buffers and
>> + * supports custom allocators.
>> + * If not set, it might not use get_buffer() or get_encoder_buffer()
>> at all, or
>> + * use operations that assume the buffer was allocated by
>> + * avcodec_default_get_buffer2 or avcodec_default_get_encoder_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..f39c8d38ce 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_encoder_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_encoder_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_encoder_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_encoder_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_encoder_buffer()\n");
>> + ret = AVERROR(EINVAL);
>> + goto fail;
>> + }
>> +
>> + ret = 0;
>> +fail:
>> + if (ret < 0) {
>> + av_log(avctx, AV_LOG_ERROR, "get_encoder_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..3192bd9e38 100644
>> --- a/libavcodec/encode.h
>> +++ b/libavcodec/encode.h
>> @@ -24,6 +24,7 @@
>> #include "libavutil/frame.h"
>> #include "avcodec.h"
>> +#include "packet.h"
>> /**
>> * Called by encoders to get the next frame for encoding.
>> @@ -36,4 +37,11 @@
>> */
>> int ff_encode_get_frame(AVCodecContext *avctx, AVFrame *frame);
>> +/**
>> + * Get a buffer for a packet. This is a wrapper around
>> + * AVCodecContext.get_encoder_buffer() and should be used instead
>> calling get_encoder_buffer()
>> + * directly.
>> + */
>> +int ff_get_encoder_buffer(AVCodecContext *avctx, AVPacket *avpkt,
>> int64_t size, int flags);
>
> Why int64_t rather than size_t?
That's what ff_alloc_packet2() uses, so i figured it will make porting
existing users much easier (basically just a sed replace, vs trying to
find if any encoder is assuming int64_t, like exrenc).
>
>> +
>> #endif /* AVCODEC_ENCODE_H */
>> diff --git a/libavcodec/options.c b/libavcodec/options.c
>> index 4bbf74ec7f..cd5fa6eb14 100644
>> --- a/libavcodec/options.c
>> +++ b/libavcodec/options.c
>> @@ -130,6 +130,7 @@ static int init_context_defaults(AVCodecContext
>> *s, const AVCodec *codec)
>> s->pkt_timebase = (AVRational){ 0, 1 };
>> s->get_buffer2 = avcodec_default_get_buffer2;
>> s->get_format = avcodec_default_get_format;
>> + s->get_encoder_buffer = avcodec_default_get_encoder_buffer;
>> s->execute = avcodec_default_execute;
>> s->execute2 = avcodec_default_execute2;
>> s->sample_aspect_ratio = (AVRational){0,1};
>>
>
> It might help to have an implementation for at least one codec and an
> example user for review.
Sure, below is huffyuvenc adapted to use this API.
> diff --git a/libavcodec/huffyuvenc.c b/libavcodec/huffyuvenc.c
> index 28a5534535..709a5a9d0e 100644
> --- a/libavcodec/huffyuvenc.c
> +++ b/libavcodec/huffyuvenc.c
> @@ -29,6 +29,7 @@
> */
>
> #include "avcodec.h"
> +#include "encode.h"
> #include "huffyuv.h"
> #include "huffman.h"
> #include "huffyuvencdsp.h"
> @@ -762,7 +763,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> const AVFrame * const p = pict;
> int i, j, size = 0, ret;
>
> - if ((ret = ff_alloc_packet2(avctx, pkt, width * height * 3 * 4 + AV_INPUT_BUFFER_MIN_SIZE, 0)) < 0)
> + if ((ret = ff_get_encoder_buffer(avctx, pkt, width * height * 3 * 4 + AV_INPUT_BUFFER_MIN_SIZE, 0)) < 0)
> return ret;
>
> if (s->context) {
> @@ -1111,7 +1112,7 @@ AVCodec ff_ffvhuff_encoder = {
> .init = encode_init,
> .encode2 = encode_frame,
> .close = encode_end,
> - .capabilities = AV_CODEC_CAP_FRAME_THREADS,
> + .capabilities = AV_CODEC_CAP_FRAME_THREADS | AV_CODEC_CAP_DR1,
> .priv_class = &ff_class,
> .pix_fmts = (const enum AVPixelFormat[]){
> AV_PIX_FMT_YUV420P, AV_PIX_FMT_YUV422P, AV_PIX_FMT_YUV444P, AV_PIX_FMT_YUV411P,
More information about the ffmpeg-devel
mailing list