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

Ronald S. Bultje rsbultje at gmail.com
Tue Oct 1 05:03:17 CEST 2013


Hi,

On Mon, Sep 30, 2013 at 8:15 AM, Derek Buitenhuis <
derek.buitenhuis at gmail.com> wrote:

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

Fixed.


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

Not sure, in ffvp8, we did it this way also, this is almost a copy of
ffvp8, except for the bw adapt changes.

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

yes :)


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

Right, the bitstream can only code 10 modes, so this is more a sanity check
than anything else.


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

Done.

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

Yes.


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

Right - 8 for refs, 1 for last frame segmentation map (part of frame
threading, will come later, it's just preparation) and 1 for the current
frame, so 10 total.


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

Not sure, Michael?


> > +#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");
>

Both done.


> > +  .capabilities          = CODEC_CAP_DR1,
>
> I assume threading is forthcoming?
>

Yes, that will come in the near future.

Ronald


More information about the ffmpeg-devel mailing list