Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Mon, May 27, 2024 at 04:32:43PM -0300, James Almer wrote:
> On 5/27/2024 4:20 PM, Michael Niedermayer wrote:
> > On Mon, May 27, 2024 at 03:17:15PM -0300, James Almer wrote:
> > > On 5/27/2024 3:11 PM, Michael Niedermayer wrote:
> > > > On Mon, May 27, 2024 at 10:15:43AM +0200, Anton Khirnov wrote:
> > > > > Quoting Michael Niedermayer (2024-04-27 02:36:23)
> > > > > > This allows detecting issues in side data related code, same as what
> > > > > > framecrc does for before already for packet data itself.
> > > > > > 
> > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > > ---
> > > > > 
> > > > > I am against this patch. Checksumming side data is a fundamentally wrong
> > > > > thing to do.
> > > > 
> > > > It, or something equivalent is neccessary for regression testing.
> > > > (and it was you who asked also for the tests i run to be part of
> > > >    fate. But here you object to it)
> > > > 
> > > > You know, not checking side data is not checking it so differences would then not be
> > > > detected allowing for unintended changes to be introduced (aka bugs)
> > > 
> > > You have seen how much code is needed to get hashing to work for all targets
> > > with some types,
> > 
> >   framecrcenc.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 73 insertions(+), 3 deletions(-)
> > 
> > 70 more lines of code, in my patch
> > 
> > If we need another 70 to handle some corner cases, no idea if we do, thats
> > still negligible
> 
> IAMF and video_enc_params. And potential future types. It's not negligible.
> 

> > 
> > 
> > > so it does feel like it's not the right thing to do.
> > 
> > I dont think i can follow that logic
> 
> Extra custom code to take into account specific side data types in order to
> output a common hash value. Do we really need to add that when we already
> have side data type specific parsing code to in ffprobe?

side data specific code doesnt belong in ffprobe

what should instead be done is to have side data parsing code where the side data
struct is

Side data can be represented in at least 4 ways
1. one or more C structs
2. a serialized bytestream
3. json data
4. a human readable key value printout

We have some of this in ffprobe, it should be closer to iamf.c/h and then
used by framecrcenc (whatever format framecrcenc uses is not important)


[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates

[-- 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".