[FFmpeg-devel] [PATCH] avformat/framecrcenc: compute the checksum for side data
James Almer
jamrial at gmail.com
Wed May 1 02:29:07 EEST 2024
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.
>
>
>> 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list