Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: List types and checksums for for side data

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Sat, Jun 07, 2025 at 11:54:03PM -0300, James Almer wrote:
> On 5/18/2025 6:23 PM, Michael Niedermayer wrote:
> > On Sun, May 18, 2025 at 11:43:14AM -0300, James Almer wrote:
> > > On 5/18/2025 9:25 AM, Michael Niedermayer wrote:
> > > > This allows detecting changes and regressions 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>
> > > > ---
> > > >    libavformat/framecrcenc.c                   |  116 +-
> > > >    tests/ref/fate/autorotate                   |    2 +-
> > > >    tests/ref/fate/cover-art-mp3-id3v2-remux    |    2 +-
> > > >    tests/ref/fate/ffmpeg-bsf-input             |   10 +-
> > > >    tests/ref/fate/ffmpeg-spec-disposition      |    2 +-
> > > >    tests/ref/fate/force_key_frames-source      |  784 ++++++------
> > > >    tests/ref/fate/force_key_frames-source-drop |   34 +-
> > > >    tests/ref/fate/force_key_frames-source-dup  | 1224 +++++++++----------
> > > >    tests/ref/fate/gapless-mp3                  |    6 +-
> > > >    tests/ref/fate/h264_redundant_pps-side_data |    2 +-
> > > >    tests/ref/fate/id3v2-priv-remux             |    2 +-
> > > >    tests/ref/fate/matroska-hdr10-plus-remux    |    2 +-
> > > >    tests/ref/fate/matroska-ogg-opus-remux      |    2 +-
> > > >    tests/ref/fate/matroska-opus-remux          |    2 +-
> > > >    tests/ref/fate/matroska-vp8-alpha-remux     |   14 +-
> > > >    tests/ref/fate/mov-cover-image              |    2 +-
> > > >    tests/ref/fate/segment-mp4-to-ts            |  250 ++--
> > > >    tests/ref/fate/shortest                     |  100 +-
> > > >    tests/ref/fate/webm-hdr10-plus-remux        |    2 +-
> > > >    tests/ref/fate/webm-webvtt-remux            |   24 +-
> > > >    20 files changed, 1346 insertions(+), 1236 deletions(-)
> > > > 
> > > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> > > > index 2ba20f3aab4..96c2a82eb2a 100644
> > > > --- a/libavformat/framecrcenc.c
> > > > +++ b/libavformat/framecrcenc.c
> > > > @@ -21,8 +21,11 @@
> > > >    #include <inttypes.h>
> > > > +#include "config.h"
> > > >    #include "libavutil/adler32.h"
> > > >    #include "libavutil/avstring.h"
> > > > +#include "libavutil/intreadwrite.h"
> > > > +#include "libavutil/hdr_dynamic_metadata.h"
> > > >    #include "libavcodec/codec_id.h"
> > > >    #include "libavcodec/codec_par.h"
> > > > @@ -48,6 +51,20 @@ static int framecrc_write_header(struct AVFormatContext *s)
> > > >        return ff_framehash_write_header(s);
> > > >    }
> > > > +static av_unused void inline bswap(char *buf, int offset, int size)
> > > > +{
> > > > +    if (size == 8) {
> > > > +        uint64_t val = AV_RN64(buf + offset);
> > > > +        AV_WN64(buf + offset, av_bswap64(val));
> > > > +    } else if (size == 4) {
> > > > +        uint32_t val = AV_RN32(buf + offset);
> > > > +        AV_WN32(buf + offset, av_bswap32(val));
> > > > +    } else if (size == 2) {
> > > > +        uint16_t val = AV_RN16(buf + offset);
> > > > +        AV_WN16(buf + offset, av_bswap16(val));
> > > > +    }
> > > > +}
> > > > +
> > > >    static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > > >    {
> > > >        uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> > > > @@ -58,11 +75,104 @@ static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > > >        if (pkt->flags != AV_PKT_FLAG_KEY)
> > > >            av_strlcatf(buf, sizeof(buf), ", F=0x%0X", pkt->flags);
> > > >        if (pkt->side_data_elems) {
> > > > +        int i;
> > > >            av_strlcatf(buf, sizeof(buf), ", S=%d", pkt->side_data_elems);
> > > > -        for (int i = 0; i < pkt->side_data_elems; i++) {
> > > > -            av_strlcatf(buf, sizeof(buf), ", %8"SIZE_SPECIFIER,
> > > > -                        pkt->side_data[i].size);
> > > > +        for (i=0; i<pkt->side_data_elems; i++) {
> > > > +            const AVPacketSideData *const sd = &pkt->side_data[i];
> > > > +            const uint8_t *data = sd->data;
> > > > +            uint32_t side_data_crc = 0;
> > > > +
> > > > +            switch (sd->type) {
> > > 
> > > Wont this potentially introduce extra work for when we add new types?
> > 
> > no, it will be less work
> > 
> > because maintaining this in a seperate branch is the identical work
> > but in addition its also the extra work due to the seperate branch.
> > and it adds complexity if one wants to bisect.
> > 
> > Every hour i have to spend maintaining this or anything
> > else in a seperate branch is an hour less i spend on the ffmpeg core
> > repository.

> You're seeing this the wrong way.

I think partly you see my position wrong and partly we disagree a bit


> You want this feature, and every day it's
> not upstreamed, it's potential extra work in your local repo. That's on you,

I do my testing for FFmpeg. Not for me. I can stop all of it and it just means
i would have more time.


[...]

> The way this should be handled is: Does the project need it, or do
> developers want it?

no, thats close but its not the correct question.
I think a better question is:
does it improve FFmpeg. Where FFmpegs value is a weighted sum of many
things like future features, future bugs, future speed, future #users,
future #developers, ...

"need" is too narrow, alot of things are good but not strictly needed
"want" is emotional and subjective, does anyone want money without working ?

Or just think of it that way. You have 2 companies, one that tries to maximize
future profits and one that does only what it needs or its members want ?
If you had to take a bet with all the money you own, in which of the 2
would you invest ?
We are not a company, but the principle is the same. For us its not profits
in dollar terms but in user satisfaction maybe.


> Because the extra work i talked about earlier is
> potentially adding a case to the switch for every new side data type added
> from now on, that you're putting on other developers' shoulders.

This is true
But also the extra tests take work off peoples sholders by recognizing bugs
related to sidedata earlier


> 
> That said, I'm not against this in principle, but again, it will mean extra
> work when adding future side data types that encapsulate long elaborate
> structs.



[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire

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