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

[Michael Niedermayer]

On Sat, May 04, 2024 at 10:54:59PM -0300, James Almer wrote:
> On 5/4/2024 10:45 PM, Michael Niedermayer wrote:
> > On Sat, May 04, 2024 at 06:02:25PM -0300, James Almer wrote:
> > > 
> > > 
> > > On 5/4/2024 5:58 PM, Michael Niedermayer wrote:
> > > > On Sat, May 04, 2024 at 12:16:00PM -0300, James Almer wrote:
> > > > > On 5/4/2024 5:34 AM, Marton Balint wrote:
> > > > > > 
> > > > > > 
> > > > > > On Thu, 2 May 2024, James Almer wrote:
> > > > > > 
> > > > > > > On 5/2/2024 6:23 PM, Marton Balint wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > >    On Wed, 1 May 2024, Michael Niedermayer wrote:
> > > > > > > > 
> > > > > > > > >    This allows detecting issues in side data related code, same as what
> > > > > > > > >    framecrc does for before already for packet data itself.
> > > > > > > > > 
> > > > > > > > >    This basically reverts c6ae560a18d67b9ddaa25a0338b7fb55e3312e57.
> > > > > > > > 
> > > > > > > >    Can you at least add an option which allows disabling dumping the side
> > > > > > > >    data? Changing the format of framecrc output again and again is
> > > > > > > > not very
> > > > > > > 
> > > > > > > The framehash/framemd5 muxer is versioned, which is what you should
> > > > > > > use if you want parseable output.
> > > > > > 
> > > > > > Okay, but then the question is that why framecrc is using different code
> > > > > > and options?
> > > > > 
> > > > > Originally it was framecrc (using AVAdler) and framemd5 (using AVMD5). The
> > > > > latter was renamed/aliased to framehash and made to use the AVHash API,
> > > > > which supports all lavu hashing algorithms, and is versioned.
> > > > > If anyone cared, framecrc could be also made into an alias of framehash that
> > > > > defaults to adler32 output, but it would result in a massive change to
> > > > > reference files, if anything because AVHash initializes adler32 with a 1
> > > > > whereas framecrc does it with a 0.
> > > > 
> > > > normally starting adler32 at 0 is bad as one could prefix by a 0 byte with
> > > > no checksum change, but given we also show the size that spcific case isnt
> > > > an issue
> > > > 
> > > > can we make the initial value for adler32 configureable so as to improve long
> > > > term stability of checksums ? (or add a adler32_1 to AVHash)
> > > 
> > > av_hash_init() already initializes adler32 with a 1. No need to do anything
> > > there. It's the framecrc muxer that uses adler32 with 0 as init value.
> > 
> > i was thinking about av_hash_init() supporting a 0 init to avoid changing every
> > checksum. Which would
> > increase git repository size
> > and make long term comparissions harder because framecrc would not be comparable
> > at all. not just some extra fields that occasionally appear
> 
> Adding AVOptions is not backwards compatible, as it would only work on
> AVHashContext from the introductory commit onwards, so the only alternative
> is probably a new init function that takes an input seed argument. Just not
> an uint32_t, as that's not extensible. It would need to be an uint8_t array.

or "adler320" in addition to "adler32"

thx

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240506/5ac37232/attachment.sig>