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

[Michael Niedermayer]

On Sun, May 18, 2025 at 09:51:16PM +0200, Michael Niedermayer wrote:
> Hi
> 
> On Sun, May 18, 2025 at 04:43:01PM +0200, Andreas Rheinhardt wrote:
> > Michael Niedermayer:
> > > 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 at 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"
> > 
> > Use proper alphabetical ordering
> 
> Is there a inproper alphabetical ordering ?
> 
> 
> 
> [...]
> 
> > 
> > > +            const AVPacketSideData *const sd = &pkt->side_data[i];
> > > +            const uint8_t *data = sd->data;
> > > +            uint32_t side_data_crc = 0;
> > > +
> > > +            switch (sd->type) {
> > > +#if HAVE_BIGENDIAN
> > > +                uint8_t bswap_buf[FFMAX3(sizeof(AVCPBProperties),
> > > +                                         sizeof(AVProducerReferenceTime),
> > > +                                         sizeof(AVDynamicHDRPlus))];
> > > +            case AV_PKT_DATA_PALETTE:
> > > +            case AV_PKT_DATA_REPLAYGAIN:
> > > +            case AV_PKT_DATA_DISPLAYMATRIX:
> > > +            case AV_PKT_DATA_STEREO3D:
> > > +            case AV_PKT_DATA_AUDIO_SERVICE_TYPE:
> > > +            case AV_PKT_DATA_FALLBACK_TRACK:
> > > +            case AV_PKT_DATA_MASTERING_DISPLAY_METADATA:
> > > +            case AV_PKT_DATA_SPHERICAL:
> > > +            case AV_PKT_DATA_CONTENT_LIGHT_LEVEL:
> > > +            case AV_PKT_DATA_S12M_TIMECODE:
> > > +                for (size_t j = 0; j < sd->size / 4; j++) {
> > > +                    uint8_t buf[4];
> > > +                    AV_WL32(buf, AV_RB32(sd->data + 4 * j));
> > > +                    side_data_crc = av_adler32_update(side_data_crc, buf, 4);
> > > +                }
> > > +                break;
> > > +            case AV_PKT_DATA_CPB_PROPERTIES:
> > > +#define BSWAP(struct, field) bswap(bswap_buf, offsetof(struct, field), sizeof(((struct){0}).field))
> > > +#define BSWAP_RAT(struct, field) do{BSWAP(struct, field.num); BSWAP(struct, field.den);}while(0)
> > 
> > Missing spaces.
> 
> Ill try to find them and tell them to return, i hope they will pick the
> right places, because iam not sure which are the right places
> 
> 
> > 
> > > +                if (sd->size == sizeof(AVCPBProperties)) {
> > > +                    memcpy(bswap_buf, sd->data, sizeof(AVCPBProperties));
> > > +                    data = bswap_buf;
> > > +                    BSWAP(AVCPBProperties, max_bitrate);
> > > +                    BSWAP(AVCPBProperties, min_bitrate);
> > > +                    BSWAP(AVCPBProperties, avg_bitrate);
> > > +                    BSWAP(AVCPBProperties, buffer_size);
> > > +                    BSWAP(AVCPBProperties, vbv_delay);
> > > +                }
> > > +                goto pod;
> > > +            case AV_PKT_DATA_PRFT:
> > > +                if (sd->size == sizeof(AVProducerReferenceTime)) {
> > > +                    memcpy(bswap_buf, sd->data, sizeof(AVProducerReferenceTime));
> > > +                    data = bswap_buf;
> > > +                    BSWAP(AVProducerReferenceTime, wallclock);
> > > +                    BSWAP(AVProducerReferenceTime, flags);
> > > +                }
> > > +                goto pod;
> > > +            case AV_PKT_DATA_DYNAMIC_HDR10_PLUS:
> > > +                if (sd->size == sizeof(AVDynamicHDRPlus)) {
> > > +                    memcpy(bswap_buf, sd->data, sizeof(AVDynamicHDRPlus));
> > > +                    data = bswap_buf;
> > > +                    for(int i = 0; i<3; i++) {
> > 
> > Missing space after for and around "<". Also, why not use FF_ARRAY_ELEMS?
> > Same below.
> 
> I could do:
> FF_ARRAY_ELEMS(((AVDynamicHDRPlus*)0)->params)

will apply the patch with these changes

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.
-------------- 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/20250607/739ad283/attachment.sig>