diff mbox series

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

Message ID d11618c98e6c9387fad53bc191dea27aa7e14341.1631006828.git.pross@xvid.org
State New
Headers show
Series [FFmpeg-devel,1/4] avcodec/siren: replace magic numbers with macro value
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Peter Ross Sept. 7, 2021, 9:28 a.m. UTC
---
 libavcodec/siren.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

Comments

Lynne Sept. 7, 2021, 9:42 a.m. UTC | #1
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.
Peter Ross Sept. 9, 2021, 8:46 a.m. UTC | #2
---

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));
Lynne Sept. 9, 2021, 10:35 a.m. UTC | #3
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.
James Almer Sept. 9, 2021, 1:40 p.m. UTC | #4
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".
>
Peter Ross Sept. 10, 2021, 9:54 a.m. UTC | #5
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)
diff mbox series

Patch

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));