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 |
Context | Check | Description |
---|---|---|
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
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)
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 [...]
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 --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; }
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(-)