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 E843D4A51A for ; Wed, 1 May 2024 00:45:58 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 66A5F68D64A; Wed, 1 May 2024 03:45:55 +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 E92E568D576 for ; Wed, 1 May 2024 03:45:48 +0300 (EEST) Received: by mail-pl1-f179.google.com with SMTP id d9443c01a7336-1e65a1370b7so59550245ad.3 for ; Tue, 30 Apr 2024 17:45:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714524346; x=1715129146; 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=ftzGiO3Yqu8iPObb8RbddM2bxOFlZbS/XMBBB0vWzXA=; b=GgqL9mdnR5lDtLKnOz13yDBTDyweG7QAs7vyBL72ChM2BHVoZ7mcr5IYaWnfXHmovs dynZEbaqG7+ms1YUbLt1lqDQ+L50X+FQnt4ZMCRYJYTZEOV4cTKqXSkTmnVy/GBv2pir bvGqYyVrG5afHVotj4aNOjpf7fck54AEDZi/qCeqcijL+f0RhqMbz/UTd6jtYkaK93JS 8dhMU3uth51TqulFvZervoumrD8+KWK/tKKb0E+uGxJHNapaYFEvOvGPls5+jqFnq7bG oyn23esQ3AYJ65oF4a3xqKiMWbP90+LEhjylI+9lwLx39n86BtZ4G3aWTIbDkjpuJ+xk OM6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714524346; x=1715129146; 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=ftzGiO3Yqu8iPObb8RbddM2bxOFlZbS/XMBBB0vWzXA=; b=b8PLhbHhG3Rtjukb74LK4iVWUMeh9AcGwx47fY+XUZhskYUOuSmDK7pBEwU344WTSs niLuD0f/sw6oftbevZt+cEVpSSZ/+VJtGpBrsaWbgfme/g0RPDIMrRaIhf2J6EiCCBNN jXhaMg0E18QmKQh9ATcv0lIAwkfWJctNRI+tHGy8VoSOA+Kyzg/WGr1mu4f7TGf3NVRB NKNsXnUhWBaa9U08M6sjguLYD1dZHo3IOdOZWsO1lzFYwG71+CesGxEpmyu0r4CqlSSw tpNoBm33XUB5EX24OIyoA35hO/BhAz6tBRvbnkASTh74o4TU6wGaU7RVgxkGw4rdHzeM yhIw== X-Gm-Message-State: AOJu0YwyVBC8AEc2CrU+8bbE1Fd+LtN8iTof/4eeTfpQnQsaOeH92POW RYngK7op+BlB8mX4UmVhpUDz2EMPpyuGIvK+AwRvYLbGkuPp+hz2FHDjDw== X-Google-Smtp-Source: AGHT+IHgBBxC3YQTuvOHejfP+BYFqUn8d45a3m4EzsjCp3fBx4n2J1QwC2MfzdkcLUtalcYnpsnj6Q== X-Received: by 2002:a17:902:eb86:b0:1eb:d79a:c111 with SMTP id q6-20020a170902eb8600b001ebd79ac111mr1231502plg.4.1714524346181; Tue, 30 Apr 2024 17:45:46 -0700 (PDT) Received: from [192.168.0.10] ([190.194.167.233]) by smtp.gmail.com with ESMTPSA id u10-20020a170903124a00b001e3d38c9e70sm23010774plh.125.2024.04.30.17.45.45 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Apr 2024 17:45:45 -0700 (PDT) Message-ID: <80af52ce-0722-4d5b-9682-01e2f5609aa9@gmail.com> Date: Tue, 30 Apr 2024 21:45:44 -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> <391e0e77-d6fb-4729-ae40-38be0d0eddc9@gmail.com> <20240501004037.GB6420@pb2> Content-Language: en-US From: James Almer In-Reply-To: <20240501004037.GB6420@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 9:40 PM, Michael Niedermayer wrote: > 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 >>>>>>> --- >>>>>>> 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. > > i think uint32 would have been better, yes It will probably not be enough anyway. There's alignment at play too. _______________________________________________ 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".