From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id B66404A4F6 for ; Tue, 30 Apr 2024 23:29:21 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C679568D63F; Wed, 1 May 2024 02:29:18 +0300 (EEST) Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0414268D52C for ; Wed, 1 May 2024 02:29:11 +0300 (EEST) Received: by mail-pl1-f175.google.com with SMTP id d9443c01a7336-1ec4dd8525cso9928585ad.3 for ; Tue, 30 Apr 2024 16:29:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714519750; x=1715124550; darn=ffmpeg.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id:from :to:cc:subject:date:message-id:reply-to; bh=QinIPt+oxepbBRgTJ1cn8dMIVufzb3havbhgHoCinN8=; b=Z7BE1X1aC3ZlNG/ZJ1eDVDnSPmXIwR4VAGfQoKPsmsjYEtf1WZWiaESp+py7ZeCGAv nheLImWdHBhU50dS52BCajTcDUN9K6f2sPsayaoTg2NzomiKDAep+u54rU9bduGDSpMS dsQhNZP86nTQSIG6K9WaZXdVT6cGzRpayGuQ0by6H/JspYcg0tzFSzr/xgpqCW1sBQUJ /4QpVXA7q71Y+vp+ew9g7YhnQPL3kTU9UtO+SkMkYcpVIXQSm2+KOiCl6dFUzZ1mLp0s uVYCiwHZN1AzXLcu8XvEu83DTxi/eMnmrm/AT9e/8bAJOHGMDH5vSZhAn+t6uJcR8g9C SWVw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714519750; x=1715124550; h=content-transfer-encoding:in-reply-to:from:content-language :references:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=QinIPt+oxepbBRgTJ1cn8dMIVufzb3havbhgHoCinN8=; b=sL87d/09wJC/HWXiFgeKBzcy2KioHDUCvq3xdOsYpN1hOhcMEeIwrbl6g9iOzeRTV7 g8LDHbr5B5JsD0gdMdR5G6EUb8hx2R39ltRByCkUl1rLZL9hWeuY9XDoaYiNXpXSwnA0 vFDx/639CEDNSAq4GaMG4+p7kvaITZGHhVCVqoSEYPN0WreVmeqiAKfcgIAhoqIzFA2X 3hxwH8hwhOV57Q9u18S5HLqkHtn4XqIVW4siF73V3t0ZwoUQ0cIpLO0UTJ2NQKehodQm bY9D3Xf2C7WHVkzoOeAcxHoMFF/hmQv8FVHZs9ccBPCQbvPp1jry54vHoj2AIGnfAEs5 OTeg== X-Gm-Message-State: AOJu0YxHIqtaQ1vcR29mPZ8Em7BVf+WrSATv6Z+LjZq/8wUyGaZWrZU/ 5iQQmJve6xOEfH09lxx/oA8rOiphhu8jmKHen1TsblUXAiR98DqcuUlFLw== X-Google-Smtp-Source: AGHT+IHNH+5hfTOGK0kRi4v5Q5v0URy2kjiKhn+AvYWqrpphQ7uAsvdvWeCHPW/V8Y9UJfgykDHaKA== X-Received: by 2002:a17:902:ed83:b0:1e0:119e:f935 with SMTP id e3-20020a170902ed8300b001e0119ef935mr762007plj.15.1714519749470; Tue, 30 Apr 2024 16:29:09 -0700 (PDT) Received: from [192.168.0.10] ([190.194.167.233]) by smtp.gmail.com with ESMTPSA id h12-20020a170902f54c00b001ebd73f61fcsm4521728plf.121.2024.04.30.16.29.08 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Apr 2024 16:29:09 -0700 (PDT) Message-ID: <391e0e77-d6fb-4729-ae40-38be0d0eddc9@gmail.com> Date: Tue, 30 Apr 2024 20:29:07 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20240427003623.118199-1-michael@niedermayer.cc> <20240427120705.GD6420@pb2> <20240430232548.GX6420@pb2> Content-Language: en-US From: James Almer In-Reply-To: <20240430232548.GX6420@pb2> Subject: Re: [FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 >>>>> --- >>>>> 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 >>>>> +#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; iside_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. > > >> but I'm against >> skipping specific types like you did above with the IAMF ones. Or rather, >> against printing 0 for them, which is misleading. I'd rather not print >> anything for them after the size. > > ok, new patch sent > > thx > > [...] > > > _______________________________________________ > 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". _______________________________________________ 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".