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

[Paul B Mahol]

On 7/30/20, Andreas Rheinhardt <andreas.rheinhardt at gmail.com> wrote:
> 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?

Because I'm lazy typing & every single time.

>
>> +    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?

Fixed.

>
>> +    bytestream2_init_writer(pby, pkt->data, pkt->size);
>> +
>
>
>>
>> +                avpriv_align_put_bits(pb);
>
> Unnecessary, flush_put_bits() already fills the bitstream with zeroes.

Fixed.

>
>> +                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).

Fixed.

>>
>> +    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.

Removed. Anyway if buffer is too small it will assert in another code.

>
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".