On Thu, May 16, 2013 at 01:09:46PM +0200, Diego Biurrun wrote:
> On Thu, May 16, 2013 at 10:54:29AM +0200, Evert Taube wrote:
> > --- /dev/null
> > +++ b/libavcodec/aic.c
> > @@ -0,0 +1,477 @@
> > +
> > + ctx->quant = src[15];
> > + ctx->interlaced = ((src[16] >> 4) == 3);
>
> pointless ()
less confusion with = and ==
> > +static void recombine_block(int16_t *dst, const uint8_t *scan,
> > + int16_t **base, int16_t **ext)
> > +{
> > + int i, j;
> > +
> > + for (i = 0; i < 4; i++) {
> > + for (j = 0; j < 4; j++)
> > + dst[scan[i * 8 + j]] = (*base)[j];
> > + for (j = 0; j < 4; j++)
> > + dst[scan[i * 8 + j + 4]] = (*ext)[j];
> > + *base += 4;
> > + *ext += 4;
> > + }
> > + for (; i < 8; i++) {
> > + for (j = 0; j < 8; j++)
> > + dst[scan[i * 8 + j]] = (*ext)[j];
> > + *ext += 8;
> > + }
> > +}
> > +
> > +static void recombine_block_il(int16_t *dst, const uint8_t *scan,
> > + int16_t **base, int16_t **ext,
> > + int block_no)
> > +{
> > + int i, j;
> > +
> > + if (block_no < 2) {
> > + for (i = 0; i < 8; i++) {
> > + for (j = 0; j < 4; j++)
> > + dst[scan[i * 8 + j]] = (*base)[j];
> > + for (j = 0; j < 4; j++)
> > + dst[scan[i * 8 + j + 4]] = (*ext)[j];
> > + *base += 4;
> > + *ext += 4;
> > + }
> > + } else {
> > + for (i = 0; i < 64; i++)
> > + dst[scan[i]] = (*ext)[i];
> > + *ext += 64;
> > + }
> > +}
>
> Maybe refactor the contents of the for-loop into a function or macro?
I don't think it will improve anything
> > + memset(ctx->slice_data, 0, sizeof(*ctx->slice_data) * slice_width
> > + * AIC_BAND_COEFFS);
>
> memset(ctx->slice_data, 0,
> sizeof(*ctx->slice_data) * slice_width * AIC_BAND_COEFFS);
>
> looks less strange IMO
will change
> > +static av_cold int aic_decode_init(AVCodecContext *avctx)
> > +{
> > + AICContext *ctx = avctx->priv_data;
> > + int i;
> > + uint8_t scan[64];
> > +
> > + ctx->avctx = avctx;
> > +
> > + avctx->pix_fmt = AV_PIX_FMT_YUV420P;
> > +
> > + ff_dsputil_init(&ctx->dsp, avctx);
> > +
> > + for (i = 0; i < 64; i++)
> > + scan[i] = i;
> > + ff_init_scantable(ctx->dsp.idct_permutation, &ctx->scantable, scan);
>
> Is that all you need dsputil for? Maybe it's time to split that part
> off off the beast...
Yes, having IDCTs and generic {get,put}_{block,pixels} in a smaller contexts
would be nice.
> > +AVCodec ff_aic_decoder = {
> > + .name = "aic",
> > + .type = AVMEDIA_TYPE_VIDEO,
> > + .id = AV_CODEC_ID_AIC,
> > + .priv_data_size = sizeof(AICContext),
> > + .init = aic_decode_init,
> > + .close = aic_decode_close,
> > + .decode = aic_decode_frame,
> > + .capabilities = CODEC_CAP_DR1,
> > + .long_name = NULL_IF_CONFIG_SMALL("Apple Intermediate Codec")
> > +};
>
> nit: Place long_name directly after name.
no codec descriptor does that IIRC
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
