support sei part 1

[nuomi2021]

based one https://github.com/ffvvc/FFmpeg/pull/257, main changes.

1. Skip commits start from https://github.com/ffvvc/FFmpeg/pull/257/commits/c922acbc39d32ca2de244aea6a7a8333aec3317c since it need more time
2. Do not allocate SEI in the FrameContext.
3. Fix hash check for RPR clips.

[nuomi2021]

@QSXW buffering_period may need more time, how about we send this to upstream for review firstly

[QSXW]

@QSXW buffering_period may need more time, how about we send this to upstream for review firstly

Sure. No worries. I copied the codes from VVC_NextSoftware and replaced the variable name, so the logic is the same and I did a lot of tests. It should be fine.

[QSXW]

@nuomi2021 Should I send the patch now?

[nuomi2021]

@nuomi2021 Should I send the patch now?

yes, please

[nuomi2021]

@QSXW buffering_period may need more time, how about we send this to upstream for review firstly

Sure. No worries. I copied the codes from VVC_NextSoftware and replaced the variable name, so the logic is the same and I did a lot of tests. It should be fine.

I am concern about https://github.com/ffvvc/FFmpeg/pull/257/commits/a02cb4ead418a6a2bbcba7c795a031209138eb18 Need more time to check, why we need to change this for SEI. :) Please send others and give me 1 more week for this.

[QSXW]

@QSXW buffering_period may need more time, how about we send this to upstream for review firstly

Sure. No worries. I copied the codes from VVC_NextSoftware and replaced the variable name, so the logic is the same and I did a lot of tests. It should be fine.

I am concern about a02cb4e Need more time to check, why we need to change this for SEI. :) Please send others and give me 1 more week for this.

No worries. Let me wait for the review.

[nuomi2021]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

[QSXW]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

[nuomi2021]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

I can check this if you tell me how to generate a crc and checksum clip. :) thank you

[QSXW]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

I can check this if you tell me how to generate a crc and checksum clip. :) thank you

It's easy to cauclated the common crc with FFmpeg API.

    crc = av_crc(av_crc_get_table(AV_CRC_24_IEEE), 0xCE04B7U, buf, buf_size);

1. get the table.
2. pass the CRC that you calculated the before. The av_crc can be called repeatedly like:

CRC = 0XFFFF
for buffer in buffers:
     CRC = av_crc(av_crc_get_table(), CRC, buf.data, buf.size)

3. You can replace the logic here

[nuomi2021]

if we can use hash, we can use a more generic way to do the hash verification.

uint8_t hash[AV_HASH_MAX_SIZE]; uint8_t target = h->md5 const char name[] = {"MD5", "CRC24", "adler32"} av_hash_alloc(&ctx, name[h->type]); for buffer in buffers: av_hash_update() av_hash_final(ctx, &hash); same = !memcpy(hash, target, av_hash_get_size(ctx);

Are you using the VTM to generate the crc and checksum clip? could you share with me? Better a small one, with no license issue. So we can checkin them to the ffmpeg fate.

thank you

[QSXW]

@nuomi2021 Try test.zip I use VTM to encode a part from an known movie, so it may have license issue. If we want to add this test to ffmpeg fate, we may need to generate a new one without license.

[nuomi2021]

If we want to add this test to ffmpeg fate, we may need to generate a new one without license.

I can do this, thank you.

[nuomi2021]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

I can check this if you tell me how to generate a crc and checksum clip. :) thank you

It's easy to cauclated the common crc with FFmpeg API.

    crc = av_crc(av_crc_get_table(AV_CRC_24_IEEE), 0xCE04B7U, buf, buf_size);

1. get the table.
2. pass the CRC that you calculated the before. The av_crc can be called repeatedly like:

CRC = 0XFFFF
for buffer in buffers:
     CRC = av_crc(av_crc_get_table(), CRC, buf.data, buf.size)

3. You can replace the logic here

could you help add the crc code on top of this pr? thank you

[QSXW]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

I can check this if you tell me how to generate a crc and checksum clip. :) thank you

It's easy to cauclated the common crc with FFmpeg API.

    crc = av_crc(av_crc_get_table(AV_CRC_24_IEEE), 0xCE04B7U, buf, buf_size);

1. get the table.
2. pass the CRC that you calculated the before. The av_crc can be called repeatedly like:

CRC = 0XFFFF
for buffer in buffers:
     CRC = av_crc(av_crc_get_table(), CRC, buf.data, buf.size)

3. You can replace the logic here

could you help add the crc code on top of this pr? thank you

I see const char *name[] = { "MD5", "CRC16_CCITT"}; is added. Is CRC16 not outputting the correct result the same as the picture hash?

[nuomi2021]

@QSXW can we use av_hash_alloc instead of hand writingcheck_md5 and check_checksum? for crc, maybe we need to add 16bits crc to hash.h. But it should be easy since we have AV_CRC_16_xxx Do you happen to know how to get crc and checksum clips?

I tried av_crc_get_table to calculate crc 16, but none of the AVCRCId produces the same result as the CRC of h274. Do you know what is the type of the CRC algorithm from h274 spec?

I can check this if you tell me how to generate a crc and checksum clip. :) thank you

It's easy to cauclated the common crc with FFmpeg API.

    crc = av_crc(av_crc_get_table(AV_CRC_24_IEEE), 0xCE04B7U, buf, buf_size);

1. get the table.
2. pass the CRC that you calculated the before. The av_crc can be called repeatedly like:

CRC = 0XFFFF
for buffer in buffers:
     CRC = av_crc(av_crc_get_table(), CRC, buf.data, buf.size)

3. You can replace the logic here

could you help add the crc code on top of this pr? thank you

I see const char *name[] = { "MD5", "CRC16_CCITT"}; is added. Is CRC16 not outputting the correct result the same as the picture hash?

Yes, I'm not familiar with this, but we may need to modify hash.c and select the appropriate CRC-16 table

[QSXW]

Hi @nuomi2021 .

Based on the poly and the initial value matching the CRC16-CCITT-False, I test the data with it but the result also doesn't match the picture hash. This CRC is found in the H.271 spec first. We may need to consult some experts on what type of this is.

Note, we need to append 2 byte zero to the picture data.

Reference: https://www.sunshine2k.de/articles/coding/crc/understanding_crc.html#ch731

[nuomi2021]

try CRC-CCITT (0x1D0F) at https://www.lammertbies.nl/comm/info/crc-calculation or CRC-AUG-CCITT at https://www.sunshine2k.de/coding/javascript/crc/crc_js.html CCITT is a part of ITU :)

all crc will append zeros see https://www.zlib.net/crc_v3.txt

[nuomi2021]

Hi @nuomi2021 .

Based on the poly and the initial value matching the CRC16-CCITT-False, I test the data with it but the result also doesn't match the picture hash. This CRC is found in the H.271 spec first. We may need to consult some experts on what type of this is.

@rdoeffinger Could you guide us on how to use crc.c to implement CRC16 in H.274? Thank you

[rdoeffinger]

Looks like the bits are in reverse for the input bytes, so you'd have to create a AV_CRC_16_CCITT_LE in the same way as there is e.g. a AV_CRC_16_ANSI_LE. Might still leave you with the bits in wrong (reverse) order in the final CRC value though.

[nuomi2021]

@rdoeffinger, thank you for the guidance. However, that's not quite it. We only need to use 0x0F1D as the initialization value. This value is the result of XOR, shift, and byte-swap operations applied to 0xFFFF Please help review https://patchwork.ffmpeg.org/project/ffmpeg/patch/[email protected]/

[nuomi2021]

@QSXW CRC fixed. please help review and send the entire pr to upstream for review.

[QSXW]

@QSXW CRC fixed. please help review and send the entire pr to upstream for review.

awesome！Will do on this weekend.

[rdoeffinger]

The doesn't seem right. First of all, the init value in the spec you showed clearly shows 0xffff as initialization value, so why should 0x0F1D be correct? Also by your description, 0x0F1D is the crc16 of two bytes of 0. Which means you can achieve the same thing in a less messy (though possibly slower) way by putting 2 bytes of 0 into the crc first (you can test with the previously mentioned online CRC calculation web pages). Assuming you are not actually somehow missing 2 bytes of 0 at the start, it would be a bit concerning if the spec does not even manage to specify the CRC it uses correctly...

[rdoeffinger]

Below is a code that reads from a file and applies the algorithm from the spec, I suggest you compare against that:

#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

#define MAX_SIZE (100 * 1024 * 1024)

int main(int argc, char *argv[])
{
    unsigned crc = 0xFFFF;
    FILE *f = fopen(argv[1], "r");
    assert(f);
    unsigned char *data = calloc(MAX_SIZE + 2, 1);
    size_t len = fread(data, 1, MAX_SIZE + 2, f);
    assert(len >= 0 && len <= MAX_SIZE);
    for (int bitIdx = 0; bitIdx < (len + 2) * 8; bitIdx++) {
        unsigned char dataByte = data[bitIdx >> 3];
        unsigned crcMsb = (crc >> 15) & 1;
        unsigned bitVal = (dataByte >> (7 - ( bitIdx & 7 ))) & 1;
        crc = (((crc << 1) + bitVal) & 0xFFFF) ^ (crcMsb * 0x1021);
    }
    printf("0x%x\n", crc);
}

[rdoeffinger]

Here is a variant that gives matching results for FFmpeg and the code in the spec. I think the spec authors did not understand CRCs and init values though, but it's easy enough to work around it. crctestav.txt

[rdoeffinger]

Surely too late to fix, but the "fix" to the spec code would be to remove the 2 byte 0 padding and change the crc update line to: crc = ((crc << 1) & 0xFFFF) ^ ((crcMsb ^ bitVal) * 0x1021);

[nuomi2021]

Hi Reimar Thank you for your code

Also by your description, 0x0F1D is the crc16 of two bytes of 0. It's 0xFF 0xFF, not two byes of 0

We can assume that the H274 is based on the CCITT ZERO algorithm. It prepends two 0xFF bytes and appends two zeros. Let's take an example where our data is 0x40, the 16-bit CRC register is initialized to 0, and the message is composed of the following sequence: 0xFF, 0xFF, 0x40, (0x0, 0x0)

When the two 0xFF bytes are shifted into the 16-bit CRC register, the register value becomes 0x1D0F.

You can see the result is matched.

[rdoeffinger]

It's 0xFF 0xFF, not two byes of 0

It's equivalent, initialize the crc to 0 and push 2 times 0xff or initialize to 0xfffff and push 2 times 0. If you select CRC16_CCITT_FALSE and put in 0x00 0x00 0x40 you get the same. Either way, I misunderstood the patch a bit, I thought you had modified the CRC itself (which would not make sense), but that's not the case, you only added a new method to hash.c I am not sure it really makes sense to go through the av_hash* API though, it adds a lot of overhead and allocation, plus it seems a bit inconsistent since you're not using the hash API for the checksum method either, but no strong opinion.

[nuomi2021]

It's 0xFF 0xFF, not two byes of 0

It's equivalent, initialize the crc to 0 and push 2 times 0xff or initialize to 0xfffff and push 2 times 0. If you select CRC16_CCITT_FALSE and put in 0x00 0x00 0x40 you get the same. Either way, I misunderstood the patch a bit, I thought you had modified the CRC itself (which would not make sense), but that's not the case, you only added a new method to hash.c I am not sure it really makes sense to go through the av_hash* API though, it adds a lot of overhead and allocation, plus it seems a bit inconsistent since you're not using the hash API for the checksum method either, but no strong opinion.

you are right, let me try how to reduce allocation.