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

Michael Niedermayer michael at niedermayer.cc
Wed May 1 03:40:37 EEST 2024


On Tue, Apr 30, 2024 at 08:29:07PM -0300, James Almer wrote:
> On 4/30/2024 8:25 PM, Michael Niedermayer wrote:
> > On Sun, Apr 28, 2024 at 12:43:50AM -0300, James Almer wrote:
> > > On 4/27/2024 9:07 AM, Michael Niedermayer wrote:
> > > > On Sat, Apr 27, 2024 at 12:44:18PM +0200, Andreas Rheinhardt wrote:
> > > > > Michael Niedermayer:
> > > > > > 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 at niedermayer.cc>
> > > > > > ---
> > > > > >    libavformat/framecrcenc.c                   |   76 +-
> > > > > >    tests/ref/fate/autorotate                   |    2 +-
> > > > > >    tests/ref/fate/cover-art-mp3-id3v2-remux    |    2 +-
> > > > > >    tests/ref/fate/ffmpeg-bsf-input             |   10 +-
> > > > > >    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/iamf-5_1-copy                |  208 ++--
> > > > > >    tests/ref/fate/iamf-5_1-demux               |  208 ++--
> > > > > >    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 +-
> > > > > >    21 files changed, 1513 insertions(+), 1443 deletions(-)
> > > > > > 
> > > > > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
> > > > > > index ce306a6c498..e71bfbd8777 100644
> > > > > > --- a/libavformat/framecrcenc.c
> > > > > > +++ b/libavformat/framecrcenc.c
> > > > > > @@ -21,8 +21,10 @@
> > > > > >    #include <inttypes.h>
> > > > > > +#include "config.h"
> > > > > >    #include "libavutil/adler32.h"
> > > > > >    #include "libavutil/avstring.h"
> > > > > > +#include "libavutil/intreadwrite.h"
> > > > > >    #include "libavcodec/codec_id.h"
> > > > > >    #include "libavcodec/codec_par.h"
> > > > > > @@ -48,6 +50,17 @@ 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));
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > >    static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt)
> > > > > >    {
> > > > > >        uint32_t crc = av_adler32_update(0, pkt->data, pkt->size);
> > > > > > @@ -58,11 +71,68 @@ 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;
> > > > > 
> > > > > This change is wrong.
> > > > > 
> > > > > >            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) {
> > > > > > +#if HAVE_BIGENDIAN
> > > > > > +                uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties),
> > > > > > +                                        sizeof(AVProducerReferenceTime))];
> > > > > > +            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))
> > > > > > +                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;
> > > > > > +            pod:
> > > > > > +#endif
> > > > > > +
> > > > > > +            default:
> > > > > > +                side_data_crc = av_adler32_update(0, data, sd->size);
> > > > > > +                break;
> > > > > > +            case AV_PKT_DATA_IAMF_MIX_GAIN_PARAM:
> > > > > > +            case AV_PKT_DATA_IAMF_DEMIXING_INFO_PARAM:
> > > > > > +            case AV_PKT_DATA_IAMF_RECON_GAIN_INFO_PARAM:
> > > > > > +                side_data_crc = 0;
> > > > > > +            }
> > > > > > +
> > > > > > +            av_strlcatf(buf, sizeof(buf), ", %8"SIZE_SPECIFIER", 0x%08"PRIx32,
> > > > > > +                        pkt->side_data[i].size, side_data_crc);
> > > > > >            }
> > > > > >        }
> > > > > >        av_strlcatf(buf, sizeof(buf), "\n");
> > > > > 
> > > > > You should mention that you are basically reverting
> > > > > c6ae560a18d67b9ddaa25a0338b7fb55e3312e57.
> > > > 
> > > > right, i didnt keep track of this as i just needed this locally
> > > > for testing to be possible and at the time didnt expect to
> > > > submit this to ffmpeg. But now 3 years later, it seems to make
> > > > sense to see if people want this maybe.
> > > > 
> > > > To test side data, it IS needed to well test side data.
> > > > But if people still want to not test side data in FFmpeg git.
> > > > I can continue to be the only one testing side data :)
> > > > 
> > > > ill repost it with a reference to c6ae560a18d67b9ddaa25a0338b7fb55e3312e57
> > > > and without the int i change, i did miss that.
> > > 
> > > I'm not against adding checksum for side data if it can be ensured it will
> > > be the same on both little and big endian, 32bit and 64bit,
> > 
> > If side data uses types that have a size that is platform dependant
> > (except pointers). Thats probably a mistake that should be corrected
> > on the next ABI bump.
> 
> size_t is used for buffer offsets on the IAMF side data types. I guess it
> could be changed to uint32_t.

i think uint32 would have been better, yes

thx

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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20240501/f1cbcd99/attachment.sig>


More information about the ffmpeg-devel mailing list