On Mon, Jan 25, 2016 at 4:51 PM, Vittorio Giovara
<vittorio.giov...@gmail.com> wrote:
> 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.
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
