diff mbox series

[FFmpeg-devel,v2] avcodec: add a get_encoder_buffer() callback to AVCodecContext

Message ID 20210221173512.3070-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] avcodec: add a get_encoder_buffer() callback to AVCodecContext
Related show

Checks

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

Commit Message

James Almer Feb. 21, 2021, 5:35 p.m. UTC
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>
---
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(-)

Comments

Lynne Feb. 21, 2021, 5:59 p.m. UTC | #1
Feb 21, 2021, 18:35 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>
> ---
> 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
> +     * - 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.
> +     *
> +     * @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);
>  } 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);
> +
>  #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};
> -- 
> 2.30.0
>
> _______________________________________________
> 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".
>

I thought I suggested "encode", but I typed "encoder", sorry :(
"encoder" looks worse. It's a nit, you don't have to send a new patch for it,
but could you change everything to "encode" sometime before you push it?
Thanks.

With that change patch LGTM.
James Almer Feb. 21, 2021, 6:02 p.m. UTC | #2
On 2/21/2021 2:59 PM, Lynne wrote:
> Feb 21, 2021, 18:35 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>
>> ---
>> 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(-)

[...]

> I thought I suggested "encode", but I typed "encoder", sorry :(
> "encoder" looks worse. It's a nit, you don't have to send a new patch for it,
> but could you change everything to "encode" sometime before you push it?

Sure.

> Thanks.
> 
> With that change patch LGTM.

Will wait a bit to see if others want to comment, then push it after 
adding an APIChanges entry and version bump.
Mark Thompson Feb. 21, 2021, 7:13 p.m. UTC | #3
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@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?  Does it have any alignment requirements?

> +     * - 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?

> +     *
> +     * @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?

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.

(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?

> +
>   #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.

Thanks,

- Mark
James Almer Feb. 21, 2021, 8 p.m. UTC | #4
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@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,
Mark Thompson Feb. 21, 2021, 8:29 p.m. UTC | #5
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 <jamrial@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.

>> 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.

>>> +     * - 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.

>>
>> (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).

Fair enough.

>>> +
>>>   #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,

(That's applying to ffvhuff only, not huffyuv.)

>>      .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,
Yeah, I guess it's actually very easy for most encoders.  Did you have a particular use-case in mind for the user side?

Thanks,

- Mark
James Almer Feb. 21, 2021, 9:04 p.m. UTC | #6
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 <jamrial@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.

> 
>>>
>>> (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).
> 
> Fair enough.
> 
>>>> +
>>>>   #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,
> 
> (That's applying to ffvhuff only, not huffyuv.)
> 
>>>      .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,
> Yeah, I guess it's actually very easy for most encoders.  Did you have a 
> particular use-case in mind for the user side?

No, it was suggested by Anton so i figured I'd implement it. It lets 
users of the new decoupled send/receive encode API provide their own 
buffers like they could with the old encode API, so the one missing 
attractive feature of the old API is now also available in the new.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> 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".
Lynne Feb. 21, 2021, 9:33 p.m. UTC | #7
Feb 21, 2021, 22:04 by jamrial@gmail.com:

> 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 <jamrial@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.
>

+1. That was one reason I wanted the flag kept.


>>>> 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.
>

I definitely think packet buffer alignment should be mandated. v210 and various
packing/unpacking codecs' SIMD depend on this.
Should have the same rules as the alignment on AVFrames.


>>>>> +     * - 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.
>>

Are there any actual devices or APIs which put a limit on the number of packets a user
can have at one point? I'm not aware of any.
I think users should handle locking themselves if they need to.
James Almer Feb. 22, 2021, 10:27 p.m. UTC | #8
On 2/21/2021 6:04 PM, James Almer wrote:
> 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 <jamrial@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.

Looked at bit into this. AVCodec->encode2() based encoders don't support 
returning EAGAIN at all, as it completely breaks the frame threading 
logic. It would require a considerable rewrite in order to re-add a task 
that didn't fail but also didn't succeed.

Non frame threading encoders could probably support it with some minimal 
changes, but i don't think suddenly letting an scenario that was until 
now guaranteed to never happen start happening (avcodec_send_frame() and 
avcodec_receive_packet() both returning EAGAIN) is a good idea. It's an 
API break.
Letting the user's custom get_encode_buffer() callback suspend the 
thread is IMO acceptable. In frame threading scenarios, the other 
threads are still working on their own packets (afaics none depends on 
the others, since it's intra only encoders only).
James Almer March 8, 2021, 8:31 p.m. UTC | #9
On 2/22/2021 7:27 PM, James Almer wrote:
> On 2/21/2021 6:04 PM, James Almer wrote:
>> 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 <jamrial@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.
> 
> Looked at bit into this. AVCodec->encode2() based encoders don't support 
> returning EAGAIN at all, as it completely breaks the frame threading 
> logic. It would require a considerable rewrite in order to re-add a task 
> that didn't fail but also didn't succeed.
> 
> Non frame threading encoders could probably support it with some minimal 
> changes, but i don't think suddenly letting an scenario that was until 
> now guaranteed to never happen start happening (avcodec_send_frame() and 
> avcodec_receive_packet() both returning EAGAIN) is a good idea. It's an 
> API break.
> Letting the user's custom get_encode_buffer() callback suspend the 
> thread is IMO acceptable. In frame threading scenarios, the other 
> threads are still working on their own packets (afaics none depends on 
> the others, since it's intra only encoders only).

Ping. I'd like to get this in.
Lynne March 8, 2021, 8:48 p.m. UTC | #10
Mar 8, 2021, 21:31 by jamrial@gmail.com:

> On 2/22/2021 7:27 PM, James Almer wrote:
>
>>
>> Looked at bit into this. AVCodec->encode2() based encoders don't support returning EAGAIN at all, as it completely breaks the frame threading logic. It would require a considerable rewrite in order to re-add a task that didn't fail but also didn't succeed.
>>
>> Non frame threading encoders could probably support it with some minimal changes, but i don't think suddenly letting an scenario that was until now guaranteed to never happen start happening (avcodec_send_frame() and avcodec_receive_packet() both returning EAGAIN) is a good idea. It's an API break.
>> Letting the user's custom get_encode_buffer() callback suspend the thread is IMO acceptable. In frame threading scenarios, the other threads are still working on their own packets (afaics none depends on the others, since it's intra only encoders only).
>>
>
> Ping. I'd like to get this in.
>

https://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276679.html
James Almer March 8, 2021, 8:54 p.m. UTC | #11
On 3/8/2021 5:48 PM, Lynne wrote:
> Mar 8, 2021, 21:31 by jamrial@gmail.com:
> 
>> On 2/22/2021 7:27 PM, James Almer wrote:
>>
>>>
>>> Looked at bit into this. AVCodec->encode2() based encoders don't support returning EAGAIN at all, as it completely breaks the frame threading logic. It would require a considerable rewrite in order to re-add a task that didn't fail but also didn't succeed.
>>>
>>> Non frame threading encoders could probably support it with some minimal changes, but i don't think suddenly letting an scenario that was until now guaranteed to never happen start happening (avcodec_send_frame() and avcodec_receive_packet() both returning EAGAIN) is a good idea. It's an API break.
>>> Letting the user's custom get_encode_buffer() callback suspend the thread is IMO acceptable. In frame threading scenarios, the other threads are still working on their own packets (afaics none depends on the others, since it's intra only encoders only).
>>>
>>
>> Ping. I'd like to get this in.
>>
> 
> https://ffmpeg.org/pipermail/ffmpeg-devel/2021-February/276679.html

You agreed with me, so i didn't think i needed to reply to that email. 
It's Mark who hasn't yet said if he's ok with it.

And regarding to the AVPacket data alignment, that's a separate 
discussion for an API change unrelated to this.
diff mbox series

Patch

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
+     * - 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.
+     *
+     * @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);
 } 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);
+
 #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};