On Sun, 06. Dec 04:09, Andreas Rheinhardt wrote: > Do this by converting big-endian side data to little endian for > checksumming. Fixes the ts-demux FATE test.
It's quite nicely done imo. Same as Michael, I enabled ts-demux test in link below and it worked fine (PPC64 qemu) https://patchwork.ffmpeg.org/project/ffmpeg/patch/20201206030934.395352-2-andreas.rheinha...@gmail.com/ > > Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> > --- > a) When commenting the #if HAVE_BIGENDIAN out, I get the same checksum > for the test in [1] as Andriy got on a real BE system; I have not done > more testing, lacking actual BE hardware. In particular, the claim about > the ts-demux FATE test is untested. > b) If side data doesn't have the expected size, then LE and BE might > still produce different results (but then there must be a bigger problem > elsewhere). > c) This code here is designed to work even after the next major version > bump when the size of some members of AVCPBProperties change. (Of course, > some FATE checksums will need to be adapted then, but for both LE and BE > in the same manner.) > > libavformat/framecrcenc.c | 61 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 56 insertions(+), 5 deletions(-) > > diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c > index f7c48779a0..390024dbe8 100644 > --- a/libavformat/framecrcenc.c > +++ b/libavformat/framecrcenc.c > @@ -21,9 +21,11 @@ > > #include <inttypes.h> > > +#include "config.h" > #include "libavutil/adler32.h" > #include "libavutil/avstring.h" > #include "libavutil/intreadwrite.h" > +#include "libavcodec/avcodec.h" > #include "avformat.h" > #include "internal.h" > > @@ -43,6 +45,19 @@ static int framecrc_write_header(struct AVFormatContext *s) > return ff_framehash_write_header(s); > } > > +#if HAVE_BIGENDIAN > +static 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)); > + } Just wondering why you decided this way with av_bswap and not AV_WLx as in the code below. > +} > +#endif > + > static int framecrc_write_packet(struct AVFormatContext *s, AVPacket *pkt) > { > uint32_t crc = av_adler32_update(0, pkt->data, pkt->size); > @@ -58,17 +73,53 @@ static int framecrc_write_packet(struct AVFormatContext > *s, AVPacket *pkt) > > 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; > - if (HAVE_BIGENDIAN && AV_PKT_DATA_PALETTE == > pkt->side_data[i].type) { > + > + switch (sd->type) { > +#if HAVE_BIGENDIAN > + uint8_t 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 (int 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); > } > - } else { > - side_data_crc = av_adler32_update(0, > - pkt->side_data[i].data, > - pkt->side_data[i].size); > + break; > + case AV_PKT_DATA_CPB_PROPERTIES: > +#define BSWAP(struct, field) bswap(buf, offsetof(struct, field), > sizeof(((struct){0}).field)) > + if (sd->size == sizeof(AVCPBProperties)) { > + memcpy(buf, sd->data, sizeof(AVCPBProperties)); > + data = 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(buf, sd->data, sizeof(AVProducerReferenceTime)); > + data = buf; > + BSWAP(AVProducerReferenceTime, wallclock); > + BSWAP(AVProducerReferenceTime, flags); > + } > + goto pod; > + pod: > +#endif > + default: > + side_data_crc = av_adler32_update(0, data, sd->size); > } > av_strlcatf(buf, sizeof(buf), ", %8d, 0x%08"PRIx32, > pkt->side_data[i].size, side_data_crc); > } Btw libavformat/hashenc.c is also doing a conversion on the palette side data. Do you think that code should be updated? -- Andriy _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
