[FFmpeg-devel,3/4] avcodec/siren: add checksum calculation

---
 libavcodec/siren.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

7 Sept 2021, 11:28 by pross@xvid.org:

> ---
>  libavcodec/siren.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/siren.c b/libavcodec/siren.c
> index ccc69aaf40..31fb9346da 100644
> --- a/libavcodec/siren.c
> +++ b/libavcodec/siren.c
> @@ -752,7 +752,34 @@ static int siren_decode(AVCodecContext *avctx, void *data,
>  frame_error = 1;
>  }
>  
> -    skip_bits(gb, s->checksum_bits);
> +    if (s->checksum_bits) {
> +        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
> +        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
> +
> +        wpf = bits_per_frame / 16;
> +
> +        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);
> +
> +        sum = 0;
> +        for (int i = 0; i < wpf - 1; i++)
> +            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
> +        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
> +        sum = (sum >> 15) ^ (sum & 0x7FFF);
> +
> +        calculated_checksum = 0;
> +        for (int i = 0; i < 4; i++) {
> +            temp1 = ChecksumTable[i] & sum;
> +            for (int j = 8; j > 0; j >>= 1) {
> +                temp2 = temp1 >> j;
> +                temp1 ^= temp2;
> +            }
> +            calculated_checksum <<= 1;
> +            calculated_checksum |= temp1 & 1;
> +        }
> +
> +        if (checksum != calculated_checksum)
> +            frame_error = 1;
> +    }
>  
>  if (frame_error) {
>  memcpy(s->imdct_in, s->backup_frame, number_of_valid_coefs * sizeof(float));
>

Good idea, but only check it if AV_EF_CRCCHECK is set in avctx->err_recognition
and only error on mismatches if AV_EF_EXPLODE is set in the same field.

---

Thanks for suggestion. I will apply in a couple days if no objections.

 libavcodec/siren.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/libavcodec/siren.c b/libavcodec/siren.c
index 2161b29a2c..40910f34e5 100644
--- a/libavcodec/siren.c
+++ b/libavcodec/siren.c
@@ -752,7 +752,37 @@ static int siren_decode(AVCodecContext *avctx, void *data,
             frame_error = 1;
     }
 
-    skip_bits(gb, s->checksum_bits);
+    if ((avctx->err_recognition & AV_EF_CRCCHECK) && s->checksum_bits) {
+        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
+        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
+
+        wpf = bits_per_frame / 16;
+
+        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);
+
+        sum = 0;
+        for (int i = 0; i < wpf - 1; i++)
+            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
+        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
+        sum = (sum >> 15) ^ (sum & 0x7FFF);
+
+        calculated_checksum = 0;
+        for (int i = 0; i < 4; i++) {
+            temp1 = ChecksumTable[i] & sum;
+            for (int j = 8; j > 0; j >>= 1) {
+                temp2 = temp1 >> j;
+                temp1 ^= temp2;
+            }
+            calculated_checksum <<= 1;
+            calculated_checksum |= temp1 & 1;
+        }
+
+        if (checksum != calculated_checksum) {
+            if (avctx->err_recognition & AV_EF_EXPLODE)
+                return AVERROR_INVALIDDATA;
+            frame_error = 1;
+        }
+    }
 
     if (frame_error) {
         memcpy(s->imdct_in, s->backup_frame, number_of_valid_coefs * sizeof(float));

9 Sept 2021, 10:46 by pross@xvid.org:

> ---
>
> Thanks for suggestion. I will apply in a couple days if no objections.
>
>  libavcodec/siren.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/siren.c b/libavcodec/siren.c
> index 2161b29a2c..40910f34e5 100644
> --- a/libavcodec/siren.c
> +++ b/libavcodec/siren.c
> @@ -752,7 +752,37 @@ static int siren_decode(AVCodecContext *avctx, void *data,
>  frame_error = 1;
>  }
>  
> -    skip_bits(gb, s->checksum_bits);
> +    if ((avctx->err_recognition & AV_EF_CRCCHECK) && s->checksum_bits) {
> +        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
> +        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
> +
> +        wpf = bits_per_frame / 16;
> +
> +        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);
> +
> +        sum = 0;
> +        for (int i = 0; i < wpf - 1; i++)
> +            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
> +        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
> +        sum = (sum >> 15) ^ (sum & 0x7FFF);
> +
> +        calculated_checksum = 0;
> +        for (int i = 0; i < 4; i++) {
> +            temp1 = ChecksumTable[i] & sum;
> +            for (int j = 8; j > 0; j >>= 1) {
> +                temp2 = temp1 >> j;
> +                temp1 ^= temp2;
> +            }
> +            calculated_checksum <<= 1;
> +            calculated_checksum |= temp1 & 1;
> +        }
> +
> +        if (checksum != calculated_checksum) {
> +            if (avctx->err_recognition & AV_EF_EXPLODE)
> +                return AVERROR_INVALIDDATA;
> +            frame_error = 1;
> +        }
> +    }
>  
>  if (frame_error) {
>  memcpy(s->imdct_in, s->backup_frame, number_of_valid_coefs * sizeof(float));
>

Could you warn the frame is corrupt if AV_EF_EXPLODE isn't set?
I always listen to music with crccheck on just so I know if a file is damaged or not.

On 9/9/2021 5:46 AM, Peter Ross wrote:
> ---
> 
> Thanks for suggestion. I will apply in a couple days if no objections.
> 
>   libavcodec/siren.c | 32 +++++++++++++++++++++++++++++++-
>   1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/siren.c b/libavcodec/siren.c
> index 2161b29a2c..40910f34e5 100644
> --- a/libavcodec/siren.c
> +++ b/libavcodec/siren.c
> @@ -752,7 +752,37 @@ static int siren_decode(AVCodecContext *avctx, void *data,
>               frame_error = 1;
>       }
>   
> -    skip_bits(gb, s->checksum_bits);
> +    if ((avctx->err_recognition & AV_EF_CRCCHECK) && s->checksum_bits) {
> +        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
> +        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
> +
> +        wpf = bits_per_frame / 16;
> +
> +        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);

Shouldn't you read this value from the getbitcontext?

> +
> +        sum = 0;
> +        for (int i = 0; i < wpf - 1; i++)
> +            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
> +        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
> +        sum = (sum >> 15) ^ (sum & 0x7FFF);
> +
> +        calculated_checksum = 0;
> +        for (int i = 0; i < 4; i++) {
> +            temp1 = ChecksumTable[i] & sum;
> +            for (int j = 8; j > 0; j >>= 1) {
> +                temp2 = temp1 >> j;
> +                temp1 ^= temp2;
> +            }
> +            calculated_checksum <<= 1;
> +            calculated_checksum |= temp1 & 1;
> +        }

What crc is this? If it's not already supported by AVCRC, could it be 
implemented?

> +
> +        if (checksum != calculated_checksum) {
> +            if (avctx->err_recognition & AV_EF_EXPLODE)
> +                return AVERROR_INVALIDDATA;
> +            frame_error = 1;
> +        }
> +    }
>   
>       if (frame_error) {
>           memcpy(s->imdct_in, s->backup_frame, number_of_valid_coefs * sizeof(float));
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>

On Thu, Sep 09, 2021 at 10:40:06AM -0300, James Almer wrote:
> On 9/9/2021 5:46 AM, Peter Ross wrote:
> > ---
> > 
> > Thanks for suggestion. I will apply in a couple days if no objections.
> > 
> >   libavcodec/siren.c | 32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/siren.c b/libavcodec/siren.c
> > index 2161b29a2c..40910f34e5 100644
> > --- a/libavcodec/siren.c
> > +++ b/libavcodec/siren.c
> > @@ -752,7 +752,37 @@ static int siren_decode(AVCodecContext *avctx, void *data,
> >               frame_error = 1;
> >       }
> > -    skip_bits(gb, s->checksum_bits);
> > +    if ((avctx->err_recognition & AV_EF_CRCCHECK) && s->checksum_bits) {
> > +        static const uint16_t ChecksumTable[4] = {0x7F80, 0x7878, 0x6666, 0x5555};
> > +        int wpf, checksum, sum, calculated_checksum, temp1, temp2;
> > +
> > +        wpf = bits_per_frame / 16;
> > +
> > +        checksum = AV_RB16(avpkt->data + (wpf - 1) * 2) & ((1 << s->checksum_bits) - 1);
> 
> Shouldn't you read this value from the getbitcontext?

yes, but...

decode_vector() infrequently overreads the getbitcontext, meaning we don't always have
the four checkbits left at the end.

the overreads also happen in the decoder library on which the FFmpeg siren codec is based
(https://github.com/kakaroto/libsiren), and are ignored there too.

i can fix this by putting 'get_bits_left(gb) - s->checksum_bits' guards throughout the
decoder. i guess that's that i will end up doing.

> > +        sum = 0;
> > +        for (int i = 0; i < wpf - 1; i++)
> > +            sum ^= AV_RB16(avpkt->data + i * 2) << (i % 15);
> > +        sum ^= (AV_RB16(avpkt->data + (wpf - 1) * 2) & ~checksum) << ((wpf - 1) % 15);
> > +        sum = (sum >> 15) ^ (sum & 0x7FFF);
> > +
> > +        calculated_checksum = 0;
> > +        for (int i = 0; i < 4; i++) {
> > +            temp1 = ChecksumTable[i] & sum;
> > +            for (int j = 8; j > 0; j >>= 1) {
> > +                temp2 = temp1 >> j;
> > +                temp1 ^= temp2;
> > +            }
> > +            calculated_checksum <<= 1;
> > +            calculated_checksum |= temp1 & 1;
> > +        }
> 
> What crc is this? If it's not already supported by AVCRC, could it be
> implemented?

no idea. it does not look like anything we have in avcrc.

the siren7 codec specification does not describe the checksum.
this appears to be added in the microsoft implementation of siren.

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)