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

[Michael Niedermayer]

Hi Traneptora

On Wed, Sep 25, 2024 at 06:52:52PM -0400, Leo Izen wrote:
> 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.

will become uint32_t


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

An old decoder encountering a file using ec=2 will likely detect every CRC protected
block as damaged and discard them, thus not decoding the file. Technically a decoder
could be smart and ignore the new unknown ec type but thats not what our decoder does


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

no


> Whether or not, it may be more readable to declare crc as a uint32_t

maybe, but that belongs in a seperate patch


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

code is different in the next revission

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240926/c079030f/attachment.sig>