[FFmpeg-devel] [libav-devel] [PATCHv3] avcodec: Cineform HD Decoder

[Vittorio Giovara]

On Sun, Jan 24, 2016 at 7:34 PM, Kieran Kunhya <kieran at kunhya.com> wrote:
> Decodes YUV 4:2:2 10-bit and RGB 12-bit files.
> Older files with more subbands, skips, Bayer, alpha not supported.
> Alpha requires addition of GBRAP12 pixel format.

Actually you could set AV_PIX_FMT_GBRAP16 and tweak the
bits_per_raw_sample, not sure about the repercussion on the users
though.

> ---
> +static av_cold int cfhd_decode_init(AVCodecContext *avctx)

is this function _decode or _init? or is it decoder_init? imho names
would be simpler with just <tag>_<action> scheme.

> +{
> +    CFHDContext *s = avctx->priv_data;
> +
> +    avctx->pix_fmt             = AV_PIX_FMT_YUV422P10;

if the decoder supports multiple pixel formats it's better not to
initialize the pixel format here, and wait until decode(). Otherwise
it's going to cause a "parameters changed" warning and reinit any
previous filter chain.

> +    avctx->bits_per_raw_sample = 10;
> +    s->avctx                   = avctx;
> +    avctx->width               = 0;
> +    avctx->height              = 0;

isn't the context mallocz anyway?

> +    return ff_cfhd_init_vlcs(s);
> +}
> +

> +static inline void filter(int16_t *output, ptrdiff_t out_stride, int16_t *low, ptrdiff_t low_stride,
> +                          int16_t *high, ptrdiff_t high_stride, int len, uint8_t clip)
> +{
> +    int16_t tmp;
> +
> +    int i;
> +    for (i = 0; i < len; i++) {
> +        if (i == 0) {
> +            tmp = (11*low[0*low_stride] - 4*low[1*low_stride] + low[2*low_stride] + 4) >> 3;
> +            output[(2*i+0)*out_stride] = (tmp + high[0*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+0)*out_stride] = av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +            tmp = ( 5*low[0*low_stride] + 4*low[1*low_stride] - low[2*low_stride] + 4) >> 3;
> +            output[(2*i+1)*out_stride] = (tmp - high[0*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+1)*out_stride] = av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +        }
> +        else if (i == len-1){

nit, spacing and new line are still off

> +            tmp = ( 5*low[i*low_stride] + 4*low[(i-1)*low_stride] - low[(i-2)*low_stride] + 4) >> 3;
> +            output[(2*i+0)*out_stride] = (tmp + high[i*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+0)*out_stride] = av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +            tmp = (11*low[i*low_stride] - 4*low[(i-1)*low_stride] + low[(i-2)*low_stride] + 4) >> 3;
> +            output[(2*i+1)*out_stride] = (tmp - high[i*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+1)*out_stride] = av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +        }
> +        else {
> +            tmp = (low[(i-1)*low_stride] - low[(i+1)*low_stride] + 4) >> 3;
> +            output[(2*i+0)*out_stride] = (tmp + low[i*low_stride] + high[i*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+0)*out_stride] = av_clip_uintp2(output[(2*i+0)*out_stride], clip);
> +
> +            tmp = (low[(i+1)*low_stride] - low[(i-1)*low_stride] + 4) >> 3;
> +            output[(2*i+1)*out_stride] = (tmp + low[i*low_stride] - high[i*high_stride]) >> 1;
> +            if (clip)
> +                output[(2*i+1)*out_stride] = av_clip_uintp2(output[(2*i+1)*out_stride], clip);
> +        }
> +    }
> +}

+1 on the dsp suggestion

> +}
> +
> +static int alloc_buffers(AVCodecContext *avctx)
> +{
> +    CFHDContext *s = avctx->priv_data;
> +    int i, j, k;
> +
> +    avctx->width = s->coded_width;
> +    avctx->height = s->coded_height;

I think ff_set_dimensions is the function you're looking for (make
sure to check its return value)

> +    avcodec_get_chroma_sub_sample(avctx->pix_fmt, &s->chroma_x_shift, &s->chroma_y_shift);

this again? :)

> +    for (i = 0; i < 3; i++) {
> +        int width = i ? avctx->width >> s->chroma_x_shift : avctx->width;
> +        int height = i ? avctx->height >> s->chroma_y_shift : avctx->height;

could these be candidates for AV_CEIL_RSHIFT?

> +        int stride = FFALIGN(width / 8, 8) * 8;
> +        int w8, h8, w4, h4, w2, h2;
> +        height = FFALIGN(height / 8, 2) * 8;
> +        s->plane[i].width = width;
> +        s->plane[i].height = height;
> +        s->plane[i].stride = stride;
> +
> +        w8 = FFALIGN(s->plane[i].width / 8, 8);
> +        h8 = FFALIGN(s->plane[i].height / 8, 2);
> +        w4 = w8 * 2;
> +        h4 = h8 * 2;
> +        w2 = w4 * 2;
> +        h2 = h4 * 2;
> +
> +        s->plane[i].idwt_buf = av_malloc(height * stride * sizeof(*s->plane[i].idwt_buf));
> +        s->plane[i].idwt_tmp = av_malloc(height * stride * sizeof(*s->plane[i].idwt_tmp));
> +        if (!s->plane[i].idwt_buf || !s->plane[i].idwt_tmp) {
> +            return AVERROR(ENOMEM);

need to av_freep both since you don't know which one failed

> +        }

> +            av_log(avctx, AV_LOG_DEBUG, "Start subband coeffs plane %i level %i codebook %i expected %i \n", s->channel_num, s->level, s->codebook, expected);
> +
> +            init_get_bits(&s->gb, gb.buffer, bytestream2_get_bytes_left(&gb) * 8);
> +            OPEN_READER(re, &s->gb);

i got bit by this too, this needs to go inside a {} block because this
macro initializes new variables, otherwise gcc will fail to compile

> +            if (!s->codebook) {
> +                for (;;) {

> +
> +            bytes = FFALIGN(FF_CEIL_RSHIFT(get_bits_count(&s->gb), 3), 4);

AV_CEIL_RSHIFT

> +            if (bytes > bytestream2_get_bytes_left(&gb)) {
> +                av_log(avctx, AV_LOG_ERROR, "Bitstream overread error \n");

the error message could be improved, since there can't be an overread
if safe bytestream2 is used

> +                ret = AVERROR(EINVAL);
> +                goto end;

> +
> +end:
> +    if (ret < 0)
> +        return ret;
> +
> +    *got_frame = 1;
> +    return avpkt->size;

avpkt->size - bytestream2_get_bytes_left(&gb) no?

> +}
> +
> +static av_cold int cfhd_close_decoder(AVCodecContext *avctx)
> +{
> +    CFHDContext *s = avctx->priv_data;
> +
> +    free_buffers(avctx);
> +
> +    if (!avctx->internal->is_copy) {
> +        ff_free_vlc(&s->vlc_9);
> +        ff_free_vlc(&s->vlc_18);
> +    }

these functions are just av_freep wrappers, you can call them without
the additional is_copy check I think

> +
> +end:
> +    if (ret < 0)
> +        ff_free_vlc(&s->vlc_9);

revising my previous suggestion, this is going to be cleaned by the
close function automatically, especially if is_copy check is removed,
you may just return ret where needed

-- 
Vittorio