[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