[FFmpeg-devel] [PATCH] vp9: use proper refcounting.

[Clément Bœsch]

On Sun, Nov 17, 2013 at 08:55:41PM -0500, Ronald S. Bultje wrote:
> Based on something similar in libav. Author is likely Anton Khirnov
> <anton at khirnov.net> but I'm not sure.

Please add a reference to the commit such as "See 97962b2 / 72ca830"

> ---
>  libavcodec/vp9.c | 92 +++++++++++++++++++++++---------------------------------
>  1 file changed, 38 insertions(+), 54 deletions(-)
> 
> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
> index 785b187..28844b9 100644
> --- a/libavcodec/vp9.c
> +++ b/libavcodec/vp9.c
> @@ -116,7 +116,7 @@ typedef struct VP9Context {
>      uint8_t refidx[3];
>      uint8_t signbias[3];
>      uint8_t varcompref[2];
> -    AVFrame *refs[8], *f, *fb[10];
> +    AVFrame *refs[8], *f;
>  
>      struct {
>          uint8_t level;
> @@ -427,8 +427,9 @@ static int decode_frame_header(AVCodecContext *ctx,
>              s->signbias[1]    = get_bits1(&s->gb);
>              s->refidx[2]      = get_bits(&s->gb, 3);
>              s->signbias[2]    = get_bits1(&s->gb);
> -            if (!s->refs[s->refidx[0]] || !s->refs[s->refidx[1]] ||
> -                !s->refs[s->refidx[2]]) {
> +            if (!s->refs[s->refidx[0]]->buf[0] ||
> +                !s->refs[s->refidx[1]]->buf[0] ||
> +                !s->refs[s->refidx[2]]->buf[0]) {
>                  av_log(ctx, AV_LOG_ERROR, "Not all references are available\n");
>                  return AVERROR_INVALIDDATA;
>              }
> @@ -3288,7 +3289,21 @@ static void adapt_probs(VP9Context *s)
>      }
>  }
>  
> -static int vp9_decode_frame(AVCodecContext *ctx, void *out_pic,
> +static av_cold int vp9_decode_free(AVCodecContext *ctx)
> +{
> +    VP9Context *s = ctx->priv_data;
> +    int i;
> +
> +    for (i = 0; i < 8; i++)
> +        av_frame_free(&s->refs[i]);
> +    av_freep(&s->above_partition_ctx);
> +    av_freep(&s->c_b);
> +
> +    return 0;
> +}
> +
> +

nit+++: double \n

> +static int vp9_decode_frame(AVCodecContext *ctx, AVFrame *frame,
>                              int *got_frame, const uint8_t *data, int size)
>  {
>      VP9Context *s = ctx->priv_data;
> @@ -3299,11 +3314,11 @@ static int vp9_decode_frame(AVCodecContext *ctx, void *out_pic,
>      if ((res = decode_frame_header(ctx, data, size, &ref)) < 0) {
>          return res;
>      } else if (res == 0) {
> -        if (!s->refs[ref]) {
> +        if (!s->refs[ref]->buf[0]) {
>              av_log(ctx, AV_LOG_ERROR, "Requested reference %d not available\n", ref);
>              return AVERROR_INVALIDDATA;
>          }
> -        if ((res = av_frame_ref(out_pic, s->refs[ref])) < 0)
> +        if ((res = av_frame_ref(frame, s->refs[ref])) < 0)
>              return res;
>          *got_frame = 1;
>          return 0;
> @@ -3311,23 +3326,7 @@ static int vp9_decode_frame(AVCodecContext *ctx, void *out_pic,
>      data += res;
>      size -= res;
>  
> -    // discard old references
> -    for (i = 0; i < 10; i++) {
> -        AVFrame *f = s->fb[i];
> -        if (f->data[0] && f != s->f &&
> -            f != s->refs[0] && f != s->refs[1] &&
> -            f != s->refs[2] && f != s->refs[3] &&
> -            f != s->refs[4] && f != s->refs[5] &&
> -            f != s->refs[6] && f != s->refs[7])
> -            av_frame_unref(f);
> -    }
> -
> -    // find unused reference
> -    for (i = 0; i < 10; i++)
> -        if (!s->fb[i]->data[0])
> -            break;
> -    av_assert0(i < 10);
> -    s->f = s->fb[i];
> +    s->f = frame;

In the original patch, s->f was unreferenced just after this. Can you
explain why it was done as such, and/or what was wrong with it?

>      if ((res = ff_get_buffer(ctx, s->f,
>                               s->refreshrefmask ? AV_GET_BUFFER_FLAG_REF : 0)) < 0)
>          return res;
[...]

Rest should be OK, assuming it was valgrind tested.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131119/8ad72e05/attachment.asc>