[FFmpeg-devel] [PATCH] Native VP9 decoder.
Derek Buitenhuis
derek.buitenhuis at gmail.com
Mon Sep 30 14:15:36 CEST 2013
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
More information about the ffmpeg-devel
mailing list