Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: Michael Niedermayer <michael@niedermayer.cc>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v3] avcodec/dovi_rpu: verify RPU data CRC32
Date: Fri, 27 Oct 2023 20:20:14 +0200
Message-ID: <20231027182014.GV3543730@pb2> (raw)
In-Reply-To: <43bfe941-5150-4692-bb6f-170b1c0ec833@gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 3696 bytes --]

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

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

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

      reply	other threads:[~2023-10-27 18:20 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-09 17:46 quietvoid
2023-10-26 12:29 ` quietvoid
2023-10-26 21:44 ` Michael Niedermayer
2023-10-27 11:46   ` quietvoid
2023-10-27 18:20     ` Michael Niedermayer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231027182014.GV3543730@pb2 \
    --to=michael@niedermayer.cc \
    --cc=ffmpeg-devel@ffmpeg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git