[FFmpeg-devel] [PATCH] Native VP9 decoder.

[Derek Buitenhuis]

On 9/21/2013 1:53 PM, Ronald S. Bultje wrote
> +static int decode_mode(AVCodecContext *ctx)
> +{

[...]

> +    return 0;
> +}

Seems it can only return 0, so should be void.

> +    do {
> +        int val, rc;
> +
> +        val = vp56_rac_get_prob_branchy(c, tp[0]); // eob
> +        eob[band][nnz][val]++;
> +        if (!val)
> +            break;
> +
> +    skip_eob:

Can the top bit be factored out, and then you can use 'continue' instea
of a goto?

> +                if (!vp56_rac_get_prob_branchy(c, tp[8])) {
> +                    if (!vp56_rac_get_prob_branchy(c, tp[9])) {
> +                        val = 11 + (vp56_rac_get_prob(c, 173) << 2) +
> +                                   (vp56_rac_get_prob(c, 148) << 1) +
> +                                    vp56_rac_get_prob(c, 140);
> +                    } else {
> +                        val = 19 + (vp56_rac_get_prob(c, 176) << 3) +
> +                                   (vp56_rac_get_prob(c, 155) << 2) +
> +                                   (vp56_rac_get_prob(c, 140) << 1) +
> +                                    vp56_rac_get_prob(c, 135);
> +                    }
> +                } else if (!vp56_rac_get_prob_branchy(c, tp[10])) {
> +                    val = 35 + (vp56_rac_get_prob(c, 180) << 4) +
> +                               (vp56_rac_get_prob(c, 157) << 3) +
> +                               (vp56_rac_get_prob(c, 141) << 2) +
> +                               (vp56_rac_get_prob(c, 134) << 1) +
> +                                vp56_rac_get_prob(c, 130);
> +                } else {
> +                    val = 67 + (vp56_rac_get_prob(c, 254) << 13) +
> +                               (vp56_rac_get_prob(c, 254) << 12) +
> +                               (vp56_rac_get_prob(c, 254) << 11) +
> +                               (vp56_rac_get_prob(c, 252) << 10) +
> +                               (vp56_rac_get_prob(c, 249) << 9) +
> +                               (vp56_rac_get_prob(c, 243) << 8) +
> +                               (vp56_rac_get_prob(c, 230) << 7) +
> +                               (vp56_rac_get_prob(c, 196) << 6) +
> +                               (vp56_rac_get_prob(c, 177) << 5) +
> +                               (vp56_rac_get_prob(c, 153) << 4) +
> +                               (vp56_rac_get_prob(c, 140) << 3) +
> +                               (vp56_rac_get_prob(c, 133) << 2) +
> +                               (vp56_rac_get_prob(c, 130) << 1) +
> +                                vp56_rac_get_prob(c, 129);

Magic numbers?


> +static av_always_inline int check_intra_mode(VP9Context *s, int mode, uint8_t **a,
> +                                             uint8_t *dst_edge, ptrdiff_t stride_edge,
> +                                             uint8_t *dst_inner, ptrdiff_t stride_inner,
> +                                             uint8_t *l, int col, int x, int w,
> +                                             int row, int y, enum TxfmMode tx,
> +                                             int p)
> +{

[...]

> +    assert(mode >= 0 && mode < 10);

I wasn't able to ascertain if mode is in any way reliant on the input file,
i.e. could a crafted or corrupt file trip this assert?

> +typedef void (*vp9_mc_func)(uint8_t *dst, ptrdiff_t dst_stride,
> +                            const uint8_t *ref, ptrdiff_t ref_stride,
> +                            int h, int mx, int my);

Move to top or header?

> +static av_always_inline void adapt_prob(uint8_t *p, unsigned ct0, unsigned ct1,
> +                                        int max_count, int update_factor)
> +{

[...]

> +    update_factor = FASTDIV(update_factor * ct, max_count);

I don't particularly like modifying argument vars, but I do not know
if we have any sort of policy on it.

> +static int vp9_decode_frame(AVCodecContext *ctx, void *out_pic,
> +                            int *got_frame, const uint8_t *data, int size)
> +{

[...]

> +    // 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);
> +    }

I assume this has run under valgrind/helgrind/tsan/asan.

> +    // find unused reference
> +    for (i = 0; i < 10; i++)
> +        if (!s->fb[i]->data[0])
> +            break;

I assume it's impossible to have all of them used.

> +static int vp9_decode_packet(AVCodecContext *avctx, void *out_pic,
> +                             int *got_frame, AVPacket *avpkt)
> +{

[...]

> +    // read superframe index - this is a collection of individual frames that
> +    // together lead to one visible frame
> +    if (size <= 0)
> +        return AVERROR_INVALIDDATA;

Is it not guaranteed to have a non-zero packet?

> +#define case_n(a, rd) \
> +                case a: \
> +                    while (n_frames--) { \
> +                        int sz = rd; \
> +                        idx += a; \
> +                        if (sz > size) \
> +                            return AVERROR_INVALIDDATA; \

av_log(avctx, AV_LOG_ERROR, "Impossible data size: %d > %d.\n", sz, size);

> +static av_cold int vp9_decode_init(AVCodecContext *ctx)
> +{

[...]

> +    for (i = 0; i < 10; i++) {
> +        s->fb[i] = av_frame_alloc();
> +        if (!s->fb[i])
> +            return AVERROR(ENOMEM);

av_log(ctx, AV_LOG_ERROR, "Cannot allocate reference frames.\n");

> +  .capabilities          = CODEC_CAP_DR1,

I assume threading is forthcoming?

Skipped over the rest, as it's all either very obviously correct, or
numbered variable soup.

- Derek