diff mbox series

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

Message ID 20240925080656.28293-1-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel] avcodec/ffv1: Implement CRC with -1 initial and final value | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer Sept. 25, 2024, 8:06 a.m. UTC
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/ffv1.h    |  1 +
 libavcodec/ffv1dec.c | 10 ++++++----
 libavcodec/ffv1enc.c | 12 +++++++++---
 3 files changed, 16 insertions(+), 7 deletions(-)

Comments

Leo Izen Sept. 25, 2024, 10:52 p.m. UTC | #1
On 9/25/24 4:06 AM, Michael Niedermayer wrote:
> Signed-off-by: Michael Niedermayer <michael@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)
Michael Niedermayer Sept. 26, 2024, 7:51 p.m. UTC | #2
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@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 Niedermayer Sept. 26, 2024, 7:56 p.m. UTC | #3
On Thu, Sep 26, 2024 at 09:51:48PM +0200, Michael Niedermayer wrote:
> 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@niedermayer.cc>
[...]
> > >           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

it seems to have the opposit effect see:
(especially the av_log())

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index b4d719a7eec..9efc926092c 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -509,11 +509,11 @@ static int read_extra_header(FFV1Context *f)
     }

     if (f->version > 2) {
-        unsigned v;
+        uint32_t v;
         v = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref,
                    f->avctx->extradata, f->avctx->extradata_size);
         if (v != f->crcref || f->avctx->extradata_size < 4) {
-            av_log(f->avctx, AV_LOG_ERROR, "CRC mismatch %X!\n", v);
+            av_log(f->avctx, AV_LOG_ERROR, "CRC mismatch %"PRIX32"!\n", v);
             return AVERROR_INVALIDDATA;
         }
         crc = AV_RB32(f->avctx->extradata + f->avctx->extradata_size - 4);
@@ -950,10 +950,10 @@ 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), f->crcref, buf_p, v);
+            uint32_t crc = av_crc(av_crc_get_table(AV_CRC_32_IEEE), f->crcref, buf_p, v);
             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);
+                av_log(f->avctx, AV_LOG_ERROR, "slice CRC mismatch %"PRIX32"!", crc);
                 if (ts != AV_NOPTS_VALUE && avctx->pkt_timebase.num) {
                     av_log(f->avctx, AV_LOG_ERROR, "at %f seconds\n", ts*av_q2d(avctx->pkt_timebase));
                 } else if (ts != AV_NOPTS_VALUE) {


[...]
diff mbox series

Patch

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;
 
     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;
         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);
+            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);
     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);
             AV_WL32(buf_p + bytes, v);
             bytes += 4;
         }