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: 5702 bytes --]

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@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)

Is that what you are asking for ?
or do you know of a prettier way ?

[...]

thx

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Whats the most studid thing your enemy could do ? Blow himself up
Whats the most studid thing you could do ? Give up your rights and
freedom because your enemy blew himself up.


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