[FFmpeg-devel] [PATCH] avcodec/ffv1: Implement CRC with -1 initial and final value

[Leo Izen]

On 9/25/24 4:06 AM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>   libavcodec/ffv1.h    |  1 +
>   libavcodec/ffv1dec.c | 10 ++++++----
>   libavcodec/ffv1enc.c | 12 +++++++++---
>   3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/ffv1.h b/libavcodec/ffv1.h
> index 9aa04529228..06125d2be5f 100644
> --- a/libavcodec/ffv1.h
> +++ b/libavcodec/ffv1.h
> @@ -118,6 +118,7 @@ typedef struct FFV1Context {
>       int64_t picture_number;
>       int key_frame;
>       ProgressFrame picture, last_picture;
> +    int crcref;

Do we require sizeof(int) == 4?
May be more readable to declare this as an int32_t anyway (or even 
uint32_t) to make it more clear that it's a 32-bit value and not a flag, 
especially considering that av_crc takes uint32_t as an argument.


>   
>       const AVFrame *cur_enc_frame;
>       int plane_count;
> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
> index 0afdeabd915..2463d2b934e 100644
> --- a/libavcodec/ffv1dec.c
> +++ b/libavcodec/ffv1dec.c
> @@ -502,15 +502,17 @@ static int read_extra_header(FFV1Context *f)
>   
>       if (f->version > 2) {
>           f->ec = get_symbol(&c, state, 0);
> +        if (f->ec >= 2)
> +            f->crcref = -1;

How backward-compatible is this? Or is it not, which is why it needs a 
new version?

>           if (f->micro_version > 2)
>               f->intra = get_symbol(&c, state, 0);
>       }
>   
>       if (f->version > 2) {
>           unsigned v;
> -        v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0,
> +        v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref,
>                      f->avctx->extradata, f->avctx->extradata_size);
> -        if (v || f->avctx->extradata_size < 4) {
> +        if (v != f->crcref || f->avctx->extradata_size < 4) {
>               av_log(f->avctx, AV_LOG_ERROR, "CRC mismatch %X!\n", v);
>               return AVERROR_INVALIDDATA;
>           }
> @@ -948,8 +950,8 @@ static int decode_frame(AVCodecContext *avctx, AVFrame *rframe,
>           buf_p -= v;
>   
>           if (f->ec) {
> -            unsigned crc = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, buf_p, v);
> -            if (crc) {
> +            unsigned crc = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref, buf_p, v);

Do we require sizeof(unsigned int) == 4?
Whether or not, it may be more readable to declare crc as a uint32_t


> +            if (crc != f->crcref) {
>                   int64_t ts = avpkt->pts != AV_NOPTS_VALUE ? avpkt->pts : avpkt->dts;
>                   av_log(f->avctx, AV_LOG_ERROR, "slice CRC mismatch %X!", crc);
>                   if (ts != AV_NOPTS_VALUE && avctx->pkt_timebase.num) {
> diff --git a/libavcodec/ffv1enc.c b/libavcodec/ffv1enc.c
> index a6f405289eb..f4bbdd9b943 100644
> --- a/libavcodec/ffv1enc.c
> +++ b/libavcodec/ffv1enc.c
> @@ -458,7 +458,7 @@ static int write_extradata(FFV1Context *f)
>       }
>   
>       f->avctx->extradata_size = ff_rac_terminate(&c, 0);
> -    v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, f->avctx->extradata, f->avctx->extradata_size);
> +    v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref, f->avctx->extradata, f->avctx->extradata_size) ^ (f->crcref&0x4964AF46);

Nitpick: should probably be (f->crcref & 0x4964af46), the space makes it 
a bit more readable.

>       AV_WL32(f->avctx->extradata + f->avctx->extradata_size, v);
>       f->avctx->extradata_size += 4;
>   
> @@ -544,7 +544,13 @@ static av_cold int encode_init(AVCodecContext *avctx)
>       }
>   
>       if (s->ec < 0) {
> -        s->ec = (s->version >= 3);
> +        if (s->version >= 4) {
> +            s->ec = 2;
> +            s->crcref = -1;
> +        } else if (s->version >= 3) {
> +            s->ec = 1;
> +        } else
> +            s->ec = 0;
>       }
>   
>       // CRC requires version 3+
> @@ -1230,7 +1236,7 @@ static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>           if (f->ec) {
>               unsigned v;
>               buf_p[bytes++] = 0;
> -            v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, buf_p, bytes);
> +            v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref, buf_p, bytes) ^ (f->crcref&0x4964AF46);

Nitpick: should probably be (f->crcref & 0x4964af46), the space makes it 
a bit more readable.

>               AV_WL32(buf_p + bytes, v);
>               bytes += 4;
>           }

- Leo Izen (Traneptora)