[FFmpeg-devel] [PATCH 2/2] avformat/framecrcenc: Make side-data checksums endian-independent

[Andriy Gelman]

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.rheinhardt@gmail.com/

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at 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