diff mbox series

[FFmpeg-devel,v3] avformat/framecrcenc: Make side-data checksums endian-independent

Message ID 20201207035609.546350-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 0dac317ba31cd10e9df26722ac96adc3f6ef3eb3
Headers show
Series [FFmpeg-devel,v3] avformat/framecrcenc: Make side-data checksums endian-independent | expand

Checks

Context Check Description
andriy/x86 warning Failed to apply patch

Commit Message

Andreas Rheinhardt Dec. 7, 2020, 3:56 a.m. UTC 
Do this by converting big-endian side data to little endian for
checksumming.

Reviewed-by: Andriy Gelman <andriy.gelman@gmail.com>
Reviewed-by: Michael Niedermayer <michael@niedermayer.cc>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
I have incorporated Michael's resp. Andriy's suggestion regarding
av_unused resp. renaming the buffer for the actual output. But I have
left hashenc.c untouched. Unfortunately one can't simply merge framecrc
into hashenc.c without changing every checksum, because the av_hash_* API
initializes its adler-32 checksums to 1, whereas framecrc initializes it
to 0. The latter seems to be wrong; if one intends to switch to the
former, then I suggest to use the framehash muxer in FATE instead of
framecrc and deprecate framecrc without modifying it and make framehash
endian-independent (essentially using the patch here); after all, other
people may use it, too. The diff would be huge (more than 70000 lines
of removal), but if that is preferred, I can do so.

 libavformat/framecrcenc.c | 59 +++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 5 deletions(-)

Comments

Anton Khirnov Dec. 13, 2020, 10:46 a.m. UTC | #1 
IMO checksumming side data contents is a bad idea and should not be done
at all. It's supposed to be an in-memory representation, which is
inherently platform-dependent - besides endianness there are potential
issues with padding or type sizes. If we want to test side data, we
should use some other mechanism for it (e.g. ffprobe).
Nicolas George Dec. 16, 2020, 7:09 p.m. UTC | #2 
Anton Khirnov (12020-12-13):
> IMO checksumming side data contents is a bad idea and should not be done
> at all. It's supposed to be an in-memory representation, which is
> inherently platform-dependent - besides endianness there are potential
> issues with padding or type sizes. If we want to test side data, we
> should use some other mechanism for it (e.g. ffprobe).

I agree with this take, but I think you are only grazing the tip of the
iceberg here.

I think than whenever we add a simple type (enum or structure) for use
as a side-data structure, frame property or similar, we should add a set
of function to perform the basic operations, with standardized names and
prototype: freeing, copy/cloning, printing, possibly checksumming, etc.

Do you not agree it would be a worthy discipline?

(Of course, if we agree on it, we need to revisit existing types to give
them these functions too.)

Regards,
Michael Niedermayer Dec. 17, 2020, 8:41 p.m. UTC | #3 
On Wed, Dec 16, 2020 at 08:09:29PM +0100, Nicolas George wrote:
> Anton Khirnov (12020-12-13):
> > IMO checksumming side data contents is a bad idea and should not be done
> > at all. It's supposed to be an in-memory representation, which is
> > inherently platform-dependent - besides endianness there are potential
> > issues with padding or type sizes. If we want to test side data, we
> > should use some other mechanism for it (e.g. ffprobe).
> 
> I agree with this take, but I think you are only grazing the tip of the
> iceberg here.
> 
> I think than whenever we add a simple type (enum or structure) for use
> as a side-data structure, frame property or similar, we should add a set
> of function to perform the basic operations, with standardized names and
> prototype: freeing, copy/cloning, printing, possibly checksumming, etc.
> 
> Do you not agree it would be a worthy discipline?
> 
> (Of course, if we agree on it, we need to revisit existing types to give
> them these functions too.)

Maybe code which can generically serialize side data into a bit exact 
bytestream would be usefull. checksumming would be a one time implementation
on top of this not a per side data one.
Also transmitting this data accross a network or other would be trivial

Thanks

[...]
Nicolas George Dec. 17, 2020, 8:45 p.m. UTC | #4 
Michael Niedermayer (12020-12-17):
> Maybe code which can generically serialize side data into a bit exact 
> bytestream would be usefull. checksumming would be a one time implementation
> on top of this not a per side data one.
> Also transmitting this data accross a network or other would be trivial

Yes, serializing to a bytestream is probably one of the standardized
function we need for every type.

And indeed, if we have it we do need checksumming.

Regards,
diff mbox series

Patch

diff --git a/libavformat/framecrcenc.c b/libavformat/framecrcenc.c
index f7c48779a0..1fbe4aa4ee 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,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,17 +71,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 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 (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(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);
             }
             av_strlcatf(buf, sizeof(buf), ", %8d, 0x%08"PRIx32, pkt->side_data[i].size, side_data_crc);
         }