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

[Kieran Kunhya]

On Mon, Jan 25, 2016 at 4:51 PM, Vittorio Giovara
<vittorio.giovara at gmail.com> wrote:
> 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.

Dunno was told to do that.

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

There are some samples which don't have a pixel format flagged and are
implicitly AV_PIX_FMT_YUV422P10.

>> +    avctx->bits_per_raw_sample = 10;
>> +    s->avctx                   = avctx;
>> +    avctx->width               = 0;
>> +    avctx->height              = 0;
>
> isn't the context mallocz anyway?

No it's allocated from the demuxer

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

Fixed this properly by freeing all the planes if the function fails

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

Guaranteed to read all tags.
>
>> +}
>> +
>> +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

Crashes with frame threads otherwise

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

If you say so.

Kieran