[FFmpeg-devel] [RFC] cineform CFHD encoder, and decoder speedup

[Andreas Rheinhardt]

Paul B Mahol:
> 
> +static int cfhd_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
> +                             const AVFrame *frame, int *got_packet)
> +{
> +    CFHDEncContext *s = avctx->priv_data;
> +    PutByteContext *pby = &s->pby;
> +    PutBitContext *pb = &s->pb;
> +    const Codebook *const cb = s->cb;
> +    const Runbook *const rb = s->rb;

Why are these part of the context and not simply on the stack although
they are reinitialized every time they are used?

> +    unsigned pos;
> +    int ret = 0;



> 
> +    ret = ff_alloc_packet2(avctx, pkt, 6 * avctx->width * avctx->height, 0);
> +    if (ret < 0)
> +        return ret;
> +

You are writing quite a lot of stuff whose length is independent of the
dimensions (E.g. you are writing 60 + 4 * s->planes before you enter the
big for-loop). Is it possible that the allocated size might be
insufficient for small dimensions (when the allocated packet is small,
yet the constant part might not be)? (Or is there some check that I
missed that rules out small dimensions?) And shouldn't you explicitly
check whether your buffer was too small?

> +    bytestream2_init_writer(pby, pkt->data, pkt->size);
> +


> 
> +                avpriv_align_put_bits(pb);

Unnecessary, flush_put_bits() already fills the bitstream with zeroes.

> +                flush_put_bits(pb);
> +                bytestream2_skip_p(pby, put_bits_count(pb) >> 3);
> +                while (bytestream2_tell_p(pby) & 3)
> +                    bytestream2_put_byte(pby, 0);

It seems to me that this loop can be an infinite loop if the buffer
turned out too small (because you are using the safe version of the
bytestream2 API and if the packet size is not divisible by four (happens
if and only if avctx->width and avctx->height are odd), then you will
not advance to an element whose distance from the beginning is divisible
by four).
> 
> +    pkt->size   = bytestream2_tell_p(pby);
> +    pkt->flags |= AV_PKT_FLAG_KEY;
> +
> +    bytestream2_seek_p(pby, 8, SEEK_SET);
> +    for (int i = 0; i < s->planes; i++)
> +        bytestream2_put_be32(pby, s->plane[i].size);
> +
> +    av_assert0((pkt->size & 3) == 0);
> +

If your buffer turned out to be too small, the above assert might be
triggered.

- Andreas