On Sun, Jan 24, 2016 at 7:34 PM, Kieran Kunhya <kie...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
