| Message ID | 20230809174657.76-1-tcChlisop0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v3] avcodec/dovi_rpu: verify RPU data CRC32 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 09/08/2023 13.46, quietvoid wrote: > The Dolby Vision RPU contains a CRC32 to validate the payload against. > The implementation is CRC32/MPEG-2. > > The CRC is only verified with the AV_EF_CRCCHECK flag. > > Signed-off-by: quietvoid <tcChlisop0@gmail.com> > --- > libavcodec/dovi_rpu.c | 46 ++++++++++++++++++++++++++++++++++++++++--- > libavcodec/dovi_rpu.h | 3 ++- > libavcodec/hevcdec.c | 3 ++- > 3 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c > index dd38936552..1dfeee7564 100644 > --- a/libavcodec/dovi_rpu.c > +++ b/libavcodec/dovi_rpu.c > @@ -22,6 +22,7 @@ > */ > > #include "libavutil/buffer.h" > +#include "libavutil/crc.h" > > #include "dovi_rpu.h" > #include "golomb.h" > @@ -191,13 +192,17 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader * > } \ > } while (0) > > -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, > + int err_recognition) > { > AVDOVIRpuDataHeader *hdr = &s->header; > GetBitContext *gb = &(GetBitContext){0}; > DOVIVdrRef *vdr; > int ret; > > + size_t actual_rpu_size; > + uint8_t trailing_zeroes = 0; > + > uint8_t nal_prefix; > uint8_t rpu_type; > uint8_t vdr_seq_info_present; > @@ -205,7 +210,22 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > uint8_t use_prev_vdr_rpu; > uint8_t use_nlq; > uint8_t profile; > - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > + > + uint32_t rpu_data_crc32; > + uint32_t computed_crc32; > + > + for (int i = rpu_size - 1; i > 0; i--) { > + if (!rpu[i]) { > + trailing_zeroes++; > + } else { > + break; > + } > + } > + > + actual_rpu_size = rpu_size - trailing_zeroes; > + > + /* Exclude trailing byte (0x80) from reader */ > + if ((ret = init_get_bits8(gb, rpu, actual_rpu_size - 1)) < 0) > return ret; > > /* RPU header, common values */ > @@ -440,7 +460,27 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > color->source_diagonal = get_bits(gb, 10); > } > > - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ > + if (!(err_recognition & AV_EF_CRCCHECK)) > + return 0; > + > + /* Skip unsupported until CRC32 */ > + skip_bits_long(gb, get_bits_left(gb) - 32); > + > + rpu_data_crc32 = get_bits_long(gb, 32); > + > + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > + -1, rpu + 1, actual_rpu_size - 6)); > + > + if (rpu_data_crc32 != computed_crc32) { > + av_log(s->logctx, AV_LOG_ERROR, > + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", > + rpu_data_crc32, computed_crc32); > + > + if (err_recognition & AV_EF_EXPLODE) > + goto fail; > + } > + > return 0; > > fail: > diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h > index f6ca5bbbc5..2b993a72c6 100644 > --- a/libavcodec/dovi_rpu.h > +++ b/libavcodec/dovi_rpu.h > @@ -77,7 +77,8 @@ void ff_dovi_update_cfg(DOVIContext *s, const AVDOVIDecoderConfigurationRecord * > * > * Returns 0 or an error code. > */ > -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size); > +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, > + int err_recognition); > > /** > * Attach the decoded AVDOVIMetadata as side data to an AVFrame. > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index df40c91ba6..81b1a84625 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -3182,7 +3182,8 @@ static int decode_nal_units(HEVCContext *s, const uint8_t *buf, int length) > return AVERROR(ENOMEM); > memcpy(s->rpu_buf->data, nal->raw_data + 2, nal->raw_size - 2); > > - ret = ff_dovi_rpu_parse(&s->dovi_ctx, nal->data + 2, nal->size - 2); > + ret = ff_dovi_rpu_parse(&s->dovi_ctx, nal->data + 2, nal->size - 2, > + s->avctx->err_recognition); > if (ret < 0) { > av_buffer_unref(&s->rpu_buf); > av_log(s->avctx, AV_LOG_WARNING, "Error parsing DOVI NAL unit.\n"); Bumping this patch.
On Wed, Aug 09, 2023 at 01:46:57PM -0400, quietvoid wrote: > The Dolby Vision RPU contains a CRC32 to validate the payload against. > The implementation is CRC32/MPEG-2. > > The CRC is only verified with the AV_EF_CRCCHECK flag. > > Signed-off-by: quietvoid <tcChlisop0@gmail.com> > --- > libavcodec/dovi_rpu.c | 46 ++++++++++++++++++++++++++++++++++++++++--- > libavcodec/dovi_rpu.h | 3 ++- > libavcodec/hevcdec.c | 3 ++- > 3 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c > index dd38936552..1dfeee7564 100644 > --- a/libavcodec/dovi_rpu.c > +++ b/libavcodec/dovi_rpu.c > @@ -22,6 +22,7 @@ > */ > > #include "libavutil/buffer.h" > +#include "libavutil/crc.h" > > #include "dovi_rpu.h" > #include "golomb.h" > @@ -191,13 +192,17 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader * > } \ > } while (0) > > -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, > + int err_recognition) > { > AVDOVIRpuDataHeader *hdr = &s->header; > GetBitContext *gb = &(GetBitContext){0}; > DOVIVdrRef *vdr; > int ret; > > + size_t actual_rpu_size; > + uint8_t trailing_zeroes = 0; > + > uint8_t nal_prefix; > uint8_t rpu_type; > uint8_t vdr_seq_info_present; > @@ -205,7 +210,22 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > uint8_t use_prev_vdr_rpu; > uint8_t use_nlq; > uint8_t profile; > - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) > + > + uint32_t rpu_data_crc32; > + uint32_t computed_crc32; > + > + for (int i = rpu_size - 1; i > 0; i--) { > + if (!rpu[i]) { > + trailing_zeroes++; > + } else { > + break; > + } > + } > + > + actual_rpu_size = rpu_size - trailing_zeroes; > + > + /* Exclude trailing byte (0x80) from reader */ > + if ((ret = init_get_bits8(gb, rpu, actual_rpu_size - 1)) < 0) > return ret; > > /* RPU header, common values */ > @@ -440,7 +460,27 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) > color->source_diagonal = get_bits(gb, 10); > } > > - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ > + if (!(err_recognition & AV_EF_CRCCHECK)) > + return 0; > + > + /* Skip unsupported until CRC32 */ > + skip_bits_long(gb, get_bits_left(gb) - 32); > + > + rpu_data_crc32 = get_bits_long(gb, 32); > + > + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > + -1, rpu + 1, actual_rpu_size - 6)); > + > + if (rpu_data_crc32 != computed_crc32) { > + av_log(s->logctx, AV_LOG_ERROR, > + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", > + rpu_data_crc32, computed_crc32); > + > + if (err_recognition & AV_EF_EXPLODE) > + goto fail; > + } (correctly designed) CRCs have the beautifull symmetry that you can merge the crc32 value into the crc computation and then a 0 means no CRC missmatch (there are many other cool properties but this one allows to simplify the code) This works too: (and is simpler) /* Skip unsupported until CRC32 */ skip_bits_long(gb, get_bits_left(gb)); /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), -1, rpu + 1, actual_rpu_size - 2)); if (computed_crc32) { av_log(s->logctx, AV_LOG_ERROR, "RPU CRC mismatch! %"PRIX32"\n", computed_crc32); if (err_recognition & AV_EF_EXPLODE) goto fail; } [...]
On 26/10/2023 17.44, Michael Niedermayer wrote: > On Wed, Aug 09, 2023 at 01:46:57PM -0400, quietvoid wrote: >> The Dolby Vision RPU contains a CRC32 to validate the payload against. >> The implementation is CRC32/MPEG-2. >> >> The CRC is only verified with the AV_EF_CRCCHECK flag. >> >> Signed-off-by: quietvoid<tcChlisop0@gmail.com> >> --- >> libavcodec/dovi_rpu.c | 46 ++++++++++++++++++++++++++++++++++++++++--- >> libavcodec/dovi_rpu.h | 3 ++- >> libavcodec/hevcdec.c | 3 ++- >> 3 files changed, 47 insertions(+), 5 deletions(-) >> >> diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c >> index dd38936552..1dfeee7564 100644 >> --- a/libavcodec/dovi_rpu.c >> +++ b/libavcodec/dovi_rpu.c >> @@ -22,6 +22,7 @@ >> */ >> >> #include "libavutil/buffer.h" >> +#include "libavutil/crc.h" >> >> #include "dovi_rpu.h" >> #include "golomb.h" >> @@ -191,13 +192,17 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader * >> } \ >> } while (0) >> >> -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, >> + int err_recognition) >> { >> AVDOVIRpuDataHeader *hdr = &s->header; >> GetBitContext *gb = &(GetBitContext){0}; >> DOVIVdrRef *vdr; >> int ret; >> >> + size_t actual_rpu_size; >> + uint8_t trailing_zeroes = 0; >> + >> uint8_t nal_prefix; >> uint8_t rpu_type; >> uint8_t vdr_seq_info_present; >> @@ -205,7 +210,22 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> uint8_t use_prev_vdr_rpu; >> uint8_t use_nlq; >> uint8_t profile; >> - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) >> + >> + uint32_t rpu_data_crc32; >> + uint32_t computed_crc32; >> + >> + for (int i = rpu_size - 1; i > 0; i--) { >> + if (!rpu[i]) { >> + trailing_zeroes++; >> + } else { >> + break; >> + } >> + } >> + >> + actual_rpu_size = rpu_size - trailing_zeroes; >> + >> + /* Exclude trailing byte (0x80) from reader */ >> + if ((ret = init_get_bits8(gb, rpu, actual_rpu_size - 1)) < 0) >> return ret; >> >> /* RPU header, common values */ >> @@ -440,7 +460,27 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) >> color->source_diagonal = get_bits(gb, 10); >> } >> >> - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ >> + if (!(err_recognition & AV_EF_CRCCHECK)) >> + return 0; >> + >> + /* Skip unsupported until CRC32 */ >> + skip_bits_long(gb, get_bits_left(gb) - 32); >> + >> + rpu_data_crc32 = get_bits_long(gb, 32); >> + >> + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ >> + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), >> + -1, rpu + 1, actual_rpu_size - 6)); >> + >> + if (rpu_data_crc32 != computed_crc32) { >> + av_log(s->logctx, AV_LOG_ERROR, >> + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", >> + rpu_data_crc32, computed_crc32); >> + >> + if (err_recognition & AV_EF_EXPLODE) >> + goto fail; >> + } > (correctly designed) CRCs have the beautifull symmetry that you can merge > the crc32 value into the crc computation and then a 0 means no CRC missmatch > (there are many other cool properties but this one allows to simplify the code) > > This works too: (and is simpler) > > /* Skip unsupported until CRC32 */ > skip_bits_long(gb, get_bits_left(gb)); > > /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > -1, rpu + 1, actual_rpu_size - 2)); > > if (computed_crc32) { > av_log(s->logctx, AV_LOG_ERROR, "RPU CRC mismatch! %"PRIX32"\n", > computed_crc32); > > if (err_recognition & AV_EF_EXPLODE) > goto fail; > } Hi Michael. I like the idea and it's a cool property. However the then printed CRC on mismatch is not a useful value, so I'm unsure if it's better to simplify here. I like having the expected CRC logged here. Thanks.
On Fri, Oct 27, 2023 at 07:46:31AM -0400, quietvoid wrote: > > On 26/10/2023 17.44, Michael Niedermayer wrote: > > On Wed, Aug 09, 2023 at 01:46:57PM -0400, quietvoid wrote: > > > The Dolby Vision RPU contains a CRC32 to validate the payload against. > > > The implementation is CRC32/MPEG-2. > > > [...] > > > - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ > > > + if (!(err_recognition & AV_EF_CRCCHECK)) > > > + return 0; > > > + > > > + /* Skip unsupported until CRC32 */ > > > + skip_bits_long(gb, get_bits_left(gb) - 32); > > > + > > > + rpu_data_crc32 = get_bits_long(gb, 32); > > > + > > > + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > > > + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > > > + -1, rpu + 1, actual_rpu_size - 6)); > > > + > > > + if (rpu_data_crc32 != computed_crc32) { > > > + av_log(s->logctx, AV_LOG_ERROR, > > > + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", > > > + rpu_data_crc32, computed_crc32); > > > + > > > + if (err_recognition & AV_EF_EXPLODE) > > > + goto fail; > > > + } > > (correctly designed) CRCs have the beautifull symmetry that you can merge > > the crc32 value into the crc computation and then a 0 means no CRC missmatch > > (there are many other cool properties but this one allows to simplify the code) > > > > This works too: (and is simpler) > > > > /* Skip unsupported until CRC32 */ > > skip_bits_long(gb, get_bits_left(gb)); > > > > /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ > > computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), > > -1, rpu + 1, actual_rpu_size - 2)); > > > > if (computed_crc32) { > > av_log(s->logctx, AV_LOG_ERROR, "RPU CRC mismatch! %"PRIX32"\n", > > computed_crc32); > > > > if (err_recognition & AV_EF_EXPLODE) > > goto fail; > > } > > Hi Michael. I like the idea and it's a cool property. > > However the then printed CRC on mismatch is not a useful value, so I'm > unsure if it's better to simplify here. teh value printed is the syndrome (https://en.wikipedia.org/wiki/Decoding_methods#Syndrome_decoding) Its not a useless value. What is it usefull for? well take 2 pieces of different and random data add a correct CRC at the end of each now both will give a 0 of course on CRC checking now flip the same bits in both blocks (whichever and howmany you want) after that very likely the CRC check will not give 0 but it will give the same value for both blocks the syndrome only depends on the damage not the data, which is kind of cool. > I like having the expected CRC logged here. how do you know that expected CRC ? i mean if theres a missmatch you _know_ there is some damage but you do not know how much or where the damage is, the "expected CRC" stored there can be the damaged and in fact even the only damaged part or maybe everything is damaged. the syndrome can be used for correcting an error if you can narrow the possible errors down enough but the CRC itself iam not so sure. Maybe one could google it in hopes to find a undamaged file but for this to work the CRC would need to be printed on undamaged files and long enough. thx [...]
diff --git a/libavcodec/dovi_rpu.c b/libavcodec/dovi_rpu.c index dd38936552..1dfeee7564 100644 --- a/libavcodec/dovi_rpu.c +++ b/libavcodec/dovi_rpu.c @@ -22,6 +22,7 @@ */ #include "libavutil/buffer.h" +#include "libavutil/crc.h" #include "dovi_rpu.h" #include "golomb.h" @@ -191,13 +192,17 @@ static inline int64_t get_se_coef(GetBitContext *gb, const AVDOVIRpuDataHeader * } \ } while (0) -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, + int err_recognition) { AVDOVIRpuDataHeader *hdr = &s->header; GetBitContext *gb = &(GetBitContext){0}; DOVIVdrRef *vdr; int ret; + size_t actual_rpu_size; + uint8_t trailing_zeroes = 0; + uint8_t nal_prefix; uint8_t rpu_type; uint8_t vdr_seq_info_present; @@ -205,7 +210,22 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) uint8_t use_prev_vdr_rpu; uint8_t use_nlq; uint8_t profile; - if ((ret = init_get_bits8(gb, rpu, rpu_size)) < 0) + + uint32_t rpu_data_crc32; + uint32_t computed_crc32; + + for (int i = rpu_size - 1; i > 0; i--) { + if (!rpu[i]) { + trailing_zeroes++; + } else { + break; + } + } + + actual_rpu_size = rpu_size - trailing_zeroes; + + /* Exclude trailing byte (0x80) from reader */ + if ((ret = init_get_bits8(gb, rpu, actual_rpu_size - 1)) < 0) return ret; /* RPU header, common values */ @@ -440,7 +460,27 @@ int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size) color->source_diagonal = get_bits(gb, 10); } - /* FIXME: verify CRC32, requires implementation of AV_CRC_32_MPEG_2 */ + if (!(err_recognition & AV_EF_CRCCHECK)) + return 0; + + /* Skip unsupported until CRC32 */ + skip_bits_long(gb, get_bits_left(gb) - 32); + + rpu_data_crc32 = get_bits_long(gb, 32); + + /* Verify CRC32, buffer excludes the prefix, CRC32 and trailing byte */ + computed_crc32 = av_bswap32(av_crc(av_crc_get_table(AV_CRC_32_IEEE), + -1, rpu + 1, actual_rpu_size - 6)); + + if (rpu_data_crc32 != computed_crc32) { + av_log(s->logctx, AV_LOG_ERROR, + "RPU CRC mismatch! Expected %"PRIu32", received %"PRIu32"\n", + rpu_data_crc32, computed_crc32); + + if (err_recognition & AV_EF_EXPLODE) + goto fail; + } + return 0; fail: diff --git a/libavcodec/dovi_rpu.h b/libavcodec/dovi_rpu.h index f6ca5bbbc5..2b993a72c6 100644 --- a/libavcodec/dovi_rpu.h +++ b/libavcodec/dovi_rpu.h @@ -77,7 +77,8 @@ void ff_dovi_update_cfg(DOVIContext *s, const AVDOVIDecoderConfigurationRecord * * * Returns 0 or an error code. */ -int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size); +int ff_dovi_rpu_parse(DOVIContext *s, const uint8_t *rpu, size_t rpu_size, + int err_recognition); /** * Attach the decoded AVDOVIMetadata as side data to an AVFrame. diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index df40c91ba6..81b1a84625 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -3182,7 +3182,8 @@ static int decode_nal_units(HEVCContext *s, const uint8_t *buf, int length) return AVERROR(ENOMEM); memcpy(s->rpu_buf->data, nal->raw_data + 2, nal->raw_size - 2); - ret = ff_dovi_rpu_parse(&s->dovi_ctx, nal->data + 2, nal->size - 2); + ret = ff_dovi_rpu_parse(&s->dovi_ctx, nal->data + 2, nal->size - 2, + s->avctx->err_recognition); if (ret < 0) { av_buffer_unref(&s->rpu_buf); av_log(s->avctx, AV_LOG_WARNING, "Error parsing DOVI NAL unit.\n");
The Dolby Vision RPU contains a CRC32 to validate the payload against. The implementation is CRC32/MPEG-2. The CRC is only verified with the AV_EF_CRCCHECK flag. Signed-off-by: quietvoid <tcChlisop0@gmail.com> --- libavcodec/dovi_rpu.c | 46 ++++++++++++++++++++++++++++++++++++++++--- libavcodec/dovi_rpu.h | 3 ++- libavcodec/hevcdec.c | 3 ++- 3 files changed, 47 insertions(+), 5 deletions(-)