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 88AA04A512 for ; Wed, 1 May 2024 00:40:48 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 35F2568D64B; Wed, 1 May 2024 03:40:45 +0300 (EEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 0FA0268D57C for ; Wed, 1 May 2024 03:40:38 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 576C3E0006 for ; Wed, 1 May 2024 00:40:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1714524038; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4H35+8mdX5HUmItuMdqrZ65eZPdy5vhCx5BoNBH7JNc=; b=es966jHvpVsk768EgA2yKtoj4eQwRg8sk1P0GGyvXqYu85EnOx/nunCCxEQ/qo2IJwWVL7 +pedLa6G3NcSuXLPu8DXPAipJbuPlxxvb1YsWVG944qzBxM5mrdjvAgbJxu3r/JMoHtuI/ MJ7d3BSdp6G5XnSSSfrsgV8BgxWHzH5P2ZJoK32wZklZ/VTJAQOT7FiTfLrup5hThcts1L zcI75tW2wqH82L/mCzo/veKGit0P1/JbU+kzPY6i/Uedkz2hBKH/9Z/0qpWYSviwUWoBJv RVLVq005WkYg9V6Y5OMEUhbySCm1n54MsnyS6Djz81z9rhSqMqYoIPBEjKpZMQ== Date: Wed, 1 May 2024 02:40:37 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240501004037.GB6420@pb2> References: <20240427003623.118199-1-michael@niedermayer.cc> <20240427120705.GD6420@pb2> <20240430232548.GX6420@pb2> <391e0e77-d6fb-4729-ae40-38be0d0eddc9@gmail.com> MIME-Version: 1.0 In-Reply-To: <391e0e77-d6fb-4729-ae40-38be0d0eddc9@gmail.com> X-GND-Sasl: michael@niedermayer.cc 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-Type: multipart/mixed; boundary="===============1451140904224838898==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============1451140904224838898== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="GVtO5olzT+zv9nqb" Content-Disposition: inline --GVtO5olzT+zv9nqb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > > > > > >=20 > > > > > > 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(-) > > > > > >=20 > > > > > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcen= c.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 AVFo= rmatContext *s) > > > > > > return ff_framehash_write_header(s); > > > > > > } > > > > > > +static av_unused void inline bswap(char *buf, int offset, int = size) > > > > > > +{ > > > > > > + if (size =3D=3D 8) { > > > > > > + uint64_t val =3D AV_RN64(buf + offset); > > > > > > + AV_WN64(buf + offset, av_bswap64(val)); > > > > > > + } else if (size =3D=3D 4) { > > > > > > + uint32_t val =3D AV_RN32(buf + offset); > > > > > > + AV_WN32(buf + offset, av_bswap32(val)); > > > > > > + } > > > > > > +} > > > > > > + > > > > > > static int framecrc_write_packet(struct AVFormatContext *s, = AVPacket *pkt) > > > > > > { > > > > > > uint32_t crc =3D av_adler32_update(0, pkt->data, pkt->si= ze); > > > > > > @@ -58,11 +71,68 @@ static int framecrc_write_packet(struct AVF= ormatContext *s, AVPacket *pkt) > > > > > > if (pkt->flags !=3D AV_PKT_FLAG_KEY) > > > > > > av_strlcatf(buf, sizeof(buf), ", F=3D0x%0X", pkt->fl= ags); > > > > > > if (pkt->side_data_elems) { > > > > > > + int i; > > > > >=20 > > > > > This change is wrong. > > > > >=20 > > > > > > av_strlcatf(buf, sizeof(buf), ", S=3D%d", pkt->side_= data_elems); > > > > > > - for (int i =3D 0; i < pkt->side_data_elems; i++) { > > > > > > - av_strlcatf(buf, sizeof(buf), ", %8"SIZE_SPECIFIER, > > > > > > - pkt->side_data[i].size); > > > > > > + for (i=3D0; iside_data_elems; i++) { > > > > > > + const AVPacketSideData *const sd =3D &pkt->side_da= ta[i]; > > > > > > + const uint8_t *data =3D sd->data; > > > > > > + uint32_t side_data_crc =3D 0; > > > > > > + > > > > > > + switch (sd->type) { > > > > > > +#if HAVE_BIGENDIAN > > > > > > + uint8_t bswap_buf[FFMAX(sizeof(AVCPBProperties= ), > > > > > > + sizeof(AVProducerRefer= enceTime))]; > > > > > > + 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 =3D 0; j < sd->size / 4; j++) { > > > > > > + uint8_t buf[4]; > > > > > > + AV_WL32(buf, AV_RB32(sd->data + 4 * j)); > > > > > > + side_data_crc =3D av_adler32_update(side_d= ata_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 =3D=3D sizeof(AVCPBProperties)) { > > > > > > + memcpy(bswap_buf, sd->data, sizeof(AVCPBPr= operties)); > > > > > > + data =3D 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 =3D=3D sizeof(AVProducerReference= Time)) { > > > > > > + memcpy(bswap_buf, sd->data, sizeof(AVProdu= cerReferenceTime)); > > > > > > + data =3D bswap_buf; > > > > > > + BSWAP(AVProducerReferenceTime, wallclock); > > > > > > + BSWAP(AVProducerReferenceTime, flags); > > > > > > + } > > > > > > + goto pod; > > > > > > + pod: > > > > > > +#endif > > > > > > + > > > > > > + default: > > > > > > + side_data_crc =3D av_adler32_update(0, data, s= d->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 =3D 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"); > > > > >=20 > > > > > You should mention that you are basically reverting > > > > > c6ae560a18d67b9ddaa25a0338b7fb55e3312e57. > > > >=20 > > > > 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. > > > >=20 > > > > 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 :) > > > >=20 > > > > ill repost it with a reference to c6ae560a18d67b9ddaa25a0338b7fb55e= 3312e57 > > > > and without the int i change, i did miss that. > > >=20 > > > 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, > >=20 > > 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. >=20 > 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 [...] --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Asymptotically faster algorithms should always be preferred if you have asymptotical amounts of data --GVtO5olzT+zv9nqb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iFwEABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZjGPhQAKCRBhHseHBAsP qyacAJME0hj6v9ORt097ixr5IwJDtdrAAJ95nY5XEMC+E9jZCUlvfCpSJKsjGQ== =quuT -----END PGP SIGNATURE----- --GVtO5olzT+zv9nqb-- --===============1451140904224838898== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ 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". --===============1451140904224838898==--