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 C6C1649A67 for ; Sun, 28 Apr 2024 03:44:02 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id CD4F868D2F9; Sun, 28 Apr 2024 06:43:59 +0300 (EEST) Received: from mail-pl1-f179.google.com (mail-pl1-f179.google.com [209.85.214.179]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 8391868C2DF for ; Sun, 28 Apr 2024 06:43:53 +0300 (EEST) Received: by mail-pl1-f179.google.com with SMTP id d9443c01a7336-1e2bbc2048eso30242865ad.3 for ; Sat, 27 Apr 2024 20:43:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714275830; x=1714880630; 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=0Pvt035jt+2rmOwkBfqPwsOfqvtqe+iVobKENVyjxcc=; b=QWLuC8tpKBI2c7Gi92pOUny49lKplCX8K6OXjO6bolOFLX6Md7kwrsSGscKDXaXFvp Vlo+4yx5wcvHGz2rcXmyl1IjaH9rdiFI6/f0ta/dIXlS76WIE3Rob+Nz78s/MO96e4tq FduYQn0hEzdSInx1zbLlZUWYBL373ywoKO+FvZMvHeA5dMnjjk3hiL5J0KAiNkmY9jBB u5/1PVvoicob0Zerr8iFkC3vzObRpXE9WyZDeeeVqATty5YuNxJqq2BvbDTn7P+afWAX SCxk19lah5/2NH4JVu3OuVwQaT+NCvCDraDSUQuU7m6RZH6ernAimyF+QGmubLvYhwPw 72dg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714275830; x=1714880630; 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=0Pvt035jt+2rmOwkBfqPwsOfqvtqe+iVobKENVyjxcc=; b=fXT8wWHdrkMSEbrj5iDX10fsWzHi3ExJasK4yoeqwTN3kjwLyylNJGnMDYtHCTTb7/ PiYhbKfstNmjw3OvlyrI+o1LMbBjTjf7WTtxlTUABpRiotRSxX7erpj31wsZimYPthhy cfUO8tGyXGb0+msxMn/DWbHy+oKCi4jPjR/AxjtwyUAlcZ6hY5osQH/h26EjQCC8uSWd lw4InjRK818imQZD+wjAf7CfnDIkHzp0A7ykVSJUm9iHBy4hbv1C28mh4As2BJR1pVIw PloDNYIEriGSMHrmohk277UF4QD15BEKyuSM9YndbHNY9b99IxnTF8UM9q8wJ7tIO/UA Bl1Q== X-Gm-Message-State: AOJu0Yxk8S3E+p16b/3snpFFO6r3FuZqTxBFrn75XhBwZz0Ilw0gWUgj SLAS0CkR0A5lSeAYpXFjmZX4EMwWXJJHOF5A+xOEqUA0BKiHgGC9RWUrpA== X-Google-Smtp-Source: AGHT+IFbFzv8HYCrT37JsFnVLtYai139m2OMGmNzw7+8/8I2VdXF8RDXbGaz4K1+7cGb12alYC6Mcg== X-Received: by 2002:a17:902:fc4d:b0:1eb:2988:549d with SMTP id me13-20020a170902fc4d00b001eb2988549dmr6927203plb.40.1714275829905; Sat, 27 Apr 2024 20:43:49 -0700 (PDT) Received: from [192.168.0.10] ([190.194.167.233]) by smtp.gmail.com with ESMTPSA id o6-20020a1709026b0600b001eab1ea2165sm5487033plk.201.2024.04.27.20.43.48 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 27 Apr 2024 20:43:49 -0700 (PDT) Message-ID: Date: Sun, 28 Apr 2024 00:43:50 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20240427003623.118199-1-michael@niedermayer.cc> <20240427120705.GD6420@pb2> Content-Language: en-US From: James Almer In-Reply-To: <20240427120705.GD6420@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/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, 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. > > 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".