FFmpeg / FFV1

The FFV1 lossless video codec specification.
Other
155 stars 35 forks source link

IEEE CRC-32 referenced in spec is not a standard IEEE CRC-32 #179

Closed dwbuiten closed 5 years ago

dwbuiten commented 5 years ago

The spec says:

   "configuration_record_crc_parity" 32 bits that are chosen so that the
   "Configuration Record" as a whole has a crc remainder of 0.
   This is equivalent to storing the crc remainder in the 32-bit parity.
   The CRC generator polynomial used is the standard IEEE CRC polynomial
   (0x104C11DB7) with initial value 0.

Same for the slice CRC info.

However, this seems to not be true - what it really is, is the result of:

av_crc(av_crc_get_table(AV_CRC_32_IEEE), 0, data, datalen)

which is not a standard IEEE CRC-32 - it does not match the results of any other CRC-32 implementation I could find, with initial value of 0.

FFmpeg's own codebase knows this, because the muxers that need actual IEEE CRC-32 with initial value of 0 all do some (undocumented) dance in which they set the initial alue to UINT32_MAX and then xor the output with UINT32_MAX. No explanation is given.

The way the spec describes it, right now, is not sufficient for actually calculating the CRC-32.

JeromeMartinez commented 5 years ago

As far as I know, and this is the reason I wrote this paragraph, "IEEE CRC polynomial" indicates the polynomial only (I provided the "normal" Polynomial representation from Wikipedia) and does not indicate the initial value (0 in this case) so I added the initial value (0), and I did not indicated any pre or post inversion because there is no inversion (my opinion was that I don't need to indicate any inversion because there is no inversion). But I don't have any official IEEE doc so I am not 100% about the standard having any initial value, pre or post inversion.

Do you mean that I should make explicit that there is no pre or post inversion?

dwbuiten commented 5 years ago

The fact that there is no pre or post conversion makes it incompatible with a standard IEEE CRC-32, as far as I know, so yes, it is very important to list - see, for example, Go's standard library implementation: https://github.com/golang/go/blob/master/src/hash/crc32/crc32_generic.go#L40-L48

dwbuiten commented 5 years ago

@michaelni @JeromeMartinez I am not sure this is enough... I am still unable to get a matching CRC out of standard CRC-32 implementations, even accounting for this. I need to maybe dig deeper... Might be endian related.

JeromeMartinez commented 5 years ago

When I compare the Go implementation and my implementation of the creation of 8- or 16-slice tables, I understand that a difference is some swap (bits and bytes?).

I am not sure about how to describe these swaps in the specs, headache :(.

I'll also try to better understand the details in the CRC computing, especially compared to the EBML one (IEEE CRC-32 too, explicitly indicated as computed in little endian).

dwbuiten commented 5 years ago

Yes, it's related to table generation - its basically the difference between av_crc_init's LE parameter, though that is rather... confusingly documented.

dwbuiten commented 5 years ago

Indeed, it's related to reflectionIt's CRC-32/MPEG-2, which is defined in ISO/IEC 13818-1 and is not reflected during table generation.

More info:

@michaelni @JeromeMartinez Maybe it would be good at add a reference?

michaelni commented 5 years ago

I think we should use inversion in v4 or later as it may slightly improve error detection with appended or prefixed 0 bytes, i will submit a pull req for that in a moment. yes not awnsering the discussion here but i wrote that pull req before seeing that there are more issues

dwbuiten commented 5 years ago

I might be mistaken, it may actually be CRC-32/MPEG-2 but with a different initial value. Either way, defining it in terms of that may be useful since right now the endianness / reflection is not documented.