[FFmpeg-devel] [PATCH] Adding Cinepak encoder (attached)

[Rl]

On Tue, Jan 14, 2014 at 05:39:56AM +0100, Michael Niedermayer wrote:
> > +typedef struct {
> > +    AVCodecContext *avctx;
> > +    unsigned char *pict_bufs[4], *strip_buf, *frame_buf;
> 
> > +    AVFrame last_frame;
> > +    AVFrame best_frame;
> > +    AVFrame scratch_frame;
> > +    AVFrame input_frame;
> 
> sizeof(AVFrame) is not part of the ABI anymore so these must be changed
> to AVFrame *... or something else

Sigh.

> > +enomem:
> > +    av_free(s->codebook_input);
> > +    av_free(s->codebook_closest);
> > +    av_free(s->strip_buf);
> > +    av_free(s->frame_buf);
> > +    av_free(s->mb);
> > +#ifdef CINEPAKENC_DEBUG
> > +    av_free(s->best_mb);
> > +#endif
> > +
> > +    for(x = 0; x < (avctx->pix_fmt == AV_PIX_FMT_RGB24 ? 4 : 3); x++)
> > +        av_free(s->pict_bufs[x]);
> 
> these should all be av_freep() to prevent stale pointers from being
> left in the struct and potentially causing double frees

Ok.

> > +                    rr = 0.2857*rr + 0.5714*gg + 0.1429*bb;
> > +                    if(      rr <   0) rr =   0;
> > +                    else if (rr > 255) rr = 255;
> > +                    scratch_pict.data[0][i1 + i2*scratch_pict.linesize[0]] = rr;
> > +                }
> > +                r /= 4; g /= 4; b /= 4;
> > +// "U"
> > +                rr = -0.1429*r - 0.2857*g + 0.4286*b;
> > +                if(      rr < -128) rr = -128;
> > +                else if (rr >  127) rr =  127;
> > +                scratch_pict.data[1][0] = rr + 128; // quantize needs unsigned
> > +// "V"
> > +                rr = 0.3571*r - 0.2857*g - 0.0714*b;
> 
> with float operations there may be slight differences in the output
> which would break a fate test

Would you suggest how these expressions should be formulated instead,
to be more robust? Using 32- or 64-bit integer arithmetics?

Regards,
Rl