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

[Andreas Rheinhardt]

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.
> 

I thought that maybe we want to some day use the BE checksum in our FATE
tests (at least for some types of side data) in which case we would have
to do a conversion for LE, hence bswap.

>> +}
>> +#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?
> 

I pondered actually merging framecrcenc into hashenc, but this wouldn't
without changing every FATE ref file resulting from framecrc, because
framecrcenc initializes the checksum to 0 (which seems to be wrong)
whereas av_hash_init() initializes adler32 checksums to 1. So I just
ignored the framehash/framemd5 muxer for now. But if it is decided that
switching everything is desirable (grepping tests/fate/ shows 1018
instances of framecrc), then I'll do it.
Also framecrc writes the packet flags (except when it is a key frame)
which I consider useful (except that they should IMO also be written for
keyframes), framehash doesn't. And the header is also slightly different.

- Andreas