[FFmpeg-devel] [PATCH] avcodec/wrapped_avframe: allocate a buffer with padding

[wm4]

On Wed, 22 Feb 2017 00:14:32 +0100
Marton Balint <cus at passwd.hu> wrote:

> This ensures that the wrapped avframe will not get reallocated later, which
> would invalidate internal references such as extended data.
> 
> Signed-off-by: Marton Balint <cus at passwd.hu>
> ---
>  libavcodec/wrapped_avframe.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c
> index 13c8d8a..1436032 100644
> --- a/libavcodec/wrapped_avframe.c
> +++ b/libavcodec/wrapped_avframe.c
> @@ -43,19 +43,31 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt,
>                       const AVFrame *frame, int *got_packet)
>  {
>      AVFrame *wrapped = av_frame_clone(frame);
> +    uint8_t *data;
> +    int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE;
>  
>      if (!wrapped)
>          return AVERROR(ENOMEM);
>  
> -    pkt->buf = av_buffer_create((uint8_t *)wrapped, sizeof(*wrapped),
> +    data = av_mallocz(size);
> +    if (!data) {
> +        av_frame_free(&wrapped);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    pkt->buf = av_buffer_create(data, size,
>                                  wrapped_avframe_release_buffer, NULL,
>                                  AV_BUFFER_FLAG_READONLY);
>      if (!pkt->buf) {
>          av_frame_free(&wrapped);
> +        av_freep(&data);
>          return AVERROR(ENOMEM);
>      }
>  
> -    pkt->data = (uint8_t *)wrapped;
> +    av_frame_move_ref((AVFrame*)data, wrapped);
> +    av_frame_free(&wrapped);

I think this could be done as just

  av_frame_ref((AVFrame*)data, frame);

without calling av_frame_clone(), but it doesn't hurt either. (Changing
it might be an optional, minor improvement.)

> +
> +    pkt->data = data;
>      pkt->size = sizeof(*wrapped);
>  
>      pkt->flags |= AV_PKT_FLAG_KEY;

Patch looks good, and I like it better than checking the codec ID.

And now something in general:

wrapped_avframe is a special-case, because it stores a pointer in what
is supposed to be just a raw byte array. So it's always possible that
AVPacket use with it breaks in tricky ways that is fine with normal
codecs. To make wrapped_avframe absolutely safe, we could either
introduce a separate AVBuffer field just for wrapped_avframe, or we
could make side-data refcounted (like with AVFrame), and store the
AVFrame in side-data, which would probably be slightly more robust.
Maybe something to consider on the next ABI bump.