[FFmpeg-devel] [PATCH] avcodec: Allow user applications to provide non padded input buffers

[wm4]

On Thu, 15 Jan 2015 01:58:40 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> This can be extended easily to skip the buffer growing for codecs which do
> not need it.
> 
> The added field or a equivalent change is needed as the AVBuffer
> size might not match the actually allocated and saftely accessable
> memory
> 
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavcodec/avcodec.h |   16 ++++++++++++++++
>  libavcodec/utils.c   |   19 ++++++++++++++++++-
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 99467bb..ed0bee0 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -3128,6 +3128,18 @@ typedef struct AVCodecContext {
>       * - decoding: set by user through AVOPtions (NO direct access)
>       */
>      char *codec_whitelist;
> +
> +    /**
> +     * Allocated input size
> +     * If this is 0 then it is assumed the user allocated size + FF_INPUT_BUFFER_PADDING_SIZE.
> +     * Note, many decoders need the padding and will copy/reallocate the buffer
> +     * if insufficient padding is provided.
> +     * Currently only supported for video.
> +     * This also requires the
> +     * - encoding: unused
> +     * - decoding: set by user, user must access through av_codec_set_allocated_input_size() (NO direct access)
> +     */
> +    int allocated_input_size;

I think this makes for a terrible API. The AVPacket should contain all
relevant data for a packet. Requiring the user to put its allocated
size in a AVCodecContext field is just ridiculous.

The next best idea would be moving this field to AVPacket. But we can't
do this, because it'd require a major ABI bump (these data structures
should really be opaque...).

We could just require the user to setup an AVBuffer with the data,
since the AVBuffer has a field telling us the exact allocated size of
the memory allocation. But it requires the user to provide full
refcounting, which is not necessarily possible.

We could probably provide an AV_BUFFER_FLAG that somehow takes care of
non-refcounted data. For example, it could copy the entire buffer if a
new reference is created. This mismatches with the AVBuffer API a bit:
as long as you own a pointer to an AVBufferRef, it's valid until you
unref it. But it might be fine in this case, because in theory the
caller of avcodec_decode_video2() owns the AVPacket and the
AVPacket.buf, so if the decoder wants to keep it for longer, it has to
create a new reference, which would trigger a copy. A similar idea, but
with the same problems, would be allowing AVBufferRef.buffer==NULL.

But really, this is all just a big mess, and I feel like the only proper
way to handle this is adding a AVPacket.allocated_size field.

(Sorry, got to reply a bit late.)

Also, in general, I'm wondering why the documentation is on these
_internal_ fields, which the user must not access. Put it on the
setter/getter instead?

>  } AVCodecContext;
>  
>  AVRational av_codec_get_pkt_timebase         (const AVCodecContext *avctx);
> @@ -3145,6 +3157,9 @@ void av_codec_set_seek_preroll(AVCodecContext *avctx, int val);
>  uint16_t *av_codec_get_chroma_intra_matrix(const AVCodecContext *avctx);
>  void av_codec_set_chroma_intra_matrix(AVCodecContext *avctx, uint16_t *val);
>  
> +int av_codec_get_allocated_input_size(const AVCodecContext *avctx);
> +void av_codec_set_allocated_input_size(AVCodecContext *avctx, int val);
> +
>  /**
>   * AVProfile.
>   */
> @@ -4131,6 +4146,7 @@ int avcodec_decode_audio4(AVCodecContext *avctx, AVFrame *frame,
>   * @warning The input buffer must be FF_INPUT_BUFFER_PADDING_SIZE larger than
>   * the actual read bytes because some optimized bitstream readers read 32 or 64
>   * bits at once and could read over the end.
> + * Also @see allocated_input_size.
>   *
>   * @warning The end of the input buffer buf should be set to 0 to ensure that
>   * no overreading happens for damaged MPEG streams.
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 1ec5cae..e835b83 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1274,6 +1274,7 @@ MAKE_ACCESSORS(AVCodecContext, codec, const AVCodecDescriptor *, codec_descripto
>  MAKE_ACCESSORS(AVCodecContext, codec, int, lowres)
>  MAKE_ACCESSORS(AVCodecContext, codec, int, seek_preroll)
>  MAKE_ACCESSORS(AVCodecContext, codec, uint16_t*, chroma_intra_matrix)
> +MAKE_ACCESSORS(AVCodecContext, codec, int, allocated_input_size)
>  
>  int av_codec_get_max_lowres(const AVCodec *codec)
>  {
> @@ -2333,6 +2334,9 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>      int ret;
>      // copy to ensure we do not change avpkt
>      AVPacket tmp = *avpkt;
> +    int did_split = 0;
> +    int needs_copy = avctx->allocated_input_size && avpkt->size &&
> +                     avctx->allocated_input_size - avpkt->size < FF_INPUT_BUFFER_PADDING_SIZE;
>  
>      if (!avctx->codec)
>          return AVERROR(EINVAL);
> @@ -2347,8 +2351,19 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
>  
>      av_frame_unref(picture);
>  
> +    if (needs_copy) {
> +        memset(&tmp, 0, sizeof(tmp));
> +        if ((ret = av_packet_ref(&tmp, avpkt)) < 0) {
> +            needs_copy = 0;
> +            goto fail;
> +        }
> +        if ((ret = av_grow_packet(&tmp, FF_INPUT_BUFFER_PADDING_SIZE)) < 0)
> +            goto fail;
> +        tmp.size -= FF_INPUT_BUFFER_PADDING_SIZE;
> +    }
> +
>      if ((avctx->codec->capabilities & CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) {
> -        int did_split = av_packet_split_side_data(&tmp);
> +        did_split = av_packet_split_side_data(&tmp);
>          ret = apply_param_change(avctx, &tmp);
>          if (ret < 0) {
>              av_log(avctx, AV_LOG_ERROR, "Error applying parameter changes.\n");
> @@ -2388,6 +2403,8 @@ fail:
>              if(ret == tmp.size)
>                  ret = avpkt->size;
>          }
> +        if (needs_copy)
> +            av_packet_unref(&tmp);
>  
>          if (*got_picture_ptr) {
>              if (!avctx->refcounted_frames) {