[FFmpeg-devel] [RFC v4] libbavcodec: add a native decoder for the Daala video codec

[Andreas Cadhalpun]

On 01.01.2016 21:16, Rostislav Pehlivanov wrote:
> Thanks for the feedback.
> Fixed all the issues you reported. The new revision is in this email.

Thanks. Unfortunately you introduced a much more serious crash problem.

Also, there is now a typo "libbavcodec" (two 'b') in the subject.

> --- /dev/null
> +++ b/libavcodec/daaladec.c
> @@ -0,0 +1,810 @@
[...]
> +static av_cold int daala_decode_init(AVCodecContext *avctx)
> +{
> +    int i, r_w, r_h, err = 0;
> +    DaalaContext *s = avctx->priv_data;
> +
> +    /* Inits a default QM, if the file isn't using the default it will be reinit */
> +    s->last_qm = 1;
> +    daala_init_qmatrix(s->pvq.qmatrix, s->pvq.qmatrix_inv, s->last_qm);
> +
> +    s->fmt = find_pix_fmt(avctx->pix_fmt);
> +    if (!s->fmt) {
> +        av_log(avctx, AV_LOG_ERROR, "Unsupported pixel format - %s!\n",
> +               av_get_pix_fmt_name(avctx->pix_fmt));
> +        return AVERROR(ENOTSUP);
> +    }
> +
> +    if (ff_daaladsp_init(&s->dsp, s->fmt->depth)) {
> +        av_log(avctx, AV_LOG_ERROR, "Unsupported bit depth - %i!\n",
> +               s->fmt->depth);
> +        return AVERROR(ENOTSUP);
> +    }
> +
> +    /* Can be turned on for 8 bit files too and the demuxer gets it, but how
> +     * the hell to actually get that flag from the demuxer to this decoder? */
> +    s->fpr = s->fmt->depth > 8 ? 1 : 0;
> +
> +    s->width  = FFALIGN(avctx->width,  DAALA_BSIZE_MAX);
> +    s->height = FFALIGN(avctx->height, DAALA_BSIZE_MAX);

There used to be a loop over i here, but now i is uninitialized.

> +    r_w = s->width  >> s->fmt->dec[i][0];
> +    r_h = s->height >> s->fmt->dec[i][1];

Thus this basically always crashes.
Therefore I can't test any further until this is resolved.

> +    s->sbn_x = s->width  >> DAALA_LOG_BSIZE_MAX;
> +    s->sbn_y = s->height >> DAALA_LOG_BSIZE_MAX;
> +
> +    /* Block sizes array */
> +    s->bsizes_stride = (s->sbn_x + 2)*DAALA_BSIZE_GRID;
> +    s->bsizes = av_malloc(sizeof(enum DaalaBsize)*s->bsizes_stride*(s->sbn_y + 2)*DAALA_BSIZE_GRID);
> +    if (!s->bsizes)
> +        return AVERROR(ENOMEM);
> +
> +    /* Like motion vectors, this could be outside the image, so shift it */
> +    s->bsizes += DAALA_BSIZE_GRID*s->bsizes_stride + DAALA_BSIZE_GRID;
> +
> +    /*  CDF init */
> +    err |= daalaent_cdf_alloc(&s->dering_cdf, 4, 2, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->q_scale_cdf, 8, 4, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->haar_bit_cdf, 3, 16, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->haar_split_cdf, 15*2*5, 16, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->skip_cdf, DAALA_NBSIZES*2, 5, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->haar_coef_cdf, 15*3*(DAALA_NBSIZES+1), 16, 128, 0, 2, 0);
> +    for (i = 0; i < s->fmt->planes; i++)
> +        err |= daalaent_cdf_alloc(&s->haar_dc_mcdf[i], DAALAENT_MODEL_TAB, 16, 64, 0, 0, 1);
> +
> +    /* PVQ CDFs */
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqcodeword_cdf, 4, 16, 128, 0, 0, 0);
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqskip_cdf, 2*(DAALA_NBSIZES-1), 7, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqtheta_gain_cdf, 2*DAALA_NBSIZES*DAALAPVQ_PARTITIONS_MAX, 16, 128, 0, 2, 0);
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqtheta_mcdf, DAALAENT_MODEL_TAB, 16, 64, 0, 0, 1);
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqgain_ref_mcdf, DAALAENT_MODEL_TAB, 16, 64, 0, 0, 1);
> +    err |= daalaent_cdf_alloc(&s->pvq.pvqgain_noref_mcdf, DAALAENT_MODEL_TAB, 16, 64, 0, 0, 1);
> +
> +    /* The allocations above are unlikely to fail so checking here is okay */
> +    if (err)
> +        goto alloc_fail;
> +
> +    /* Arrays */
> +    DAALA_ALLOC_2D_ARRAY_GOTO(s->dering, s->sbn_x, s->sbn_y, uint8_t, alloc_fail);
> +    DAALA_ALLOC_2D_ARRAY_GOTO(s->q_scale, s->sbn_x, s->sbn_y, uint8_t, alloc_fail);
> +    DAALA_ALLOC_STATIC_2D_ARRAY_GOTO(s->haar_dc_buf, s->fmt->planes, s->sbn_x,
> +                                     s->sbn_y, dctcoef, alloc_fail);
> +
> +    /* Coefficients, still too much */
> +    s->lcoef    = av_malloc(r_w*r_h*sizeof(dctcoef));

Here r_w and r_h are used, but they depend on i and previously there was an array of coefs...
It seems you wanted to eliminate that, but something went quite wrong.

> +    if (!s->lcoef)
> +        goto alloc_fail;
> +
> +    s->dcoef[0] = av_malloc(r_w*r_h*sizeof(dctcoef));
> +    if (!s->dcoef[0])
> +        goto alloc_fail;
> +
> +    s->dcoef[1] = av_malloc(r_w*r_h*sizeof(dctcoef));
> +    s->dcoef[3] = s->dcoef[2] = s->dcoef[1];
> +    if (!s->dcoef[1])
> +        goto alloc_fail;
> +
> +    for (i = 0; i < s->fmt->planes; i++) {
> +        s->ccoef[i] = av_malloc(r_w*r_h*sizeof(dctcoef));
> +        if (!s->ccoef[i])
> +            goto alloc_fail;
> +    }
> +
> +    return 0;
> +
> +alloc_fail:
> +    av_log(avctx, AV_LOG_ERROR, "Failed to allocate memory!\n");
> +    daala_decode_free(avctx);
> +    return AVERROR(ENOMEM);
> +}
> +
> +AVCodec ff_daala_decoder = {
> +    .name           = "daala",
> +    .long_name      = NULL_IF_CONFIG_SMALL("Daala"),
> +    .type           = AVMEDIA_TYPE_VIDEO,
> +    .id             = AV_CODEC_ID_DAALA,
> +    .priv_data_size = sizeof(DaalaContext),
> +    .init           = daala_decode_init,
> +    .close          = daala_decode_free,
> +    .decode         = daala_decode_frame,

Please add:
    .capabilities   = AV_CODEC_CAP_EXPERIMENTAL,

At least until non-I-frames are supported.

Happy new year,
Andreas