diff mbox series

[FFmpeg-devel] avcodec/mpegaudiodec_template: Check CRCs for layer1 and layer2

Message ID 20200803163122.10983-1-michael@niedermayer.cc
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/mpegaudiodec_template: Check CRCs for layer1 and layer2
Related show

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Michael Niedermayer Aug. 3, 2020, 4:31 p.m. UTC
This differs from the MPEG specification as the actual real world
files do compute their CRC over variable areas and not the fixed
ones listed in the specification. This is also the reason for
the complexity of this code and the need to perform the CRC
check for layer2 in the middle of layer2 decoding.

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/mpegaudiodec_template.c | 61 +++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 17 deletions(-)

Comments

Lynne Aug. 3, 2020, 6:53 p.m. UTC | #1
Aug 3, 2020, 18:31 by michael@niedermayer.cc:

> This differs from the MPEG specification as the actual real world
> files do compute their CRC over variable areas and not the fixed
> ones listed in the specification. This is also the reason for
> the complexity of this code and the need to perform the CRC
> check for layer2 in the middle of layer2 decoding.
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/mpegaudiodec_template.c | 61 +++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 17 deletions(-)
>
> diff --git a/libavcodec/mpegaudiodec_template.c b/libavcodec/mpegaudiodec_template.c
> index 3d7e3ba4f2..d84e4f1cb6 100644
> --- a/libavcodec/mpegaudiodec_template.c
> +++ b/libavcodec/mpegaudiodec_template.c
> @@ -89,6 +89,7 @@ typedef struct MPADecodeContext {
>  MPADSPContext mpadsp;
>  AVFloatDSPContext *fdsp;
>  AVFrame *frame;
> +    int crc;
>

Make this a uint32_t, its what we always store CRCs as even if they're 8 bits.
Thought it was a flag on first read.



>  } MPADecodeContext;
>  
>  #define HEADER_SIZE 4
> @@ -500,12 +501,43 @@ static void imdct12(INTFLOAT *out, SUINTFLOAT *in)
>  out[11] = in0 + in5;
>  }
>  
> +static int handle_crc(MPADecodeContext *s, int sec_len)
> +{
> +    if (s->error_protection && (s->err_recognition & AV_EF_CRCCHECK)) {
> +        const uint8_t *buf = s->gb.buffer - HEADER_SIZE;
> +        int sec_byte_len  = sec_len >> 3;
> +        int sec_rem_bits  = sec_len & 7;
> +        const AVCRC *crc_tab = av_crc_get_table(AV_CRC_16_ANSI);
> +        uint8_t tmp_buf[4];
> +        uint32_t crc_val = av_crc(crc_tab, UINT16_MAX, &buf[2], 2);
> +        crc_val = av_crc(crc_tab, crc_val, &buf[6], sec_byte_len);
> +
> +        AV_WB32(tmp_buf,
> +            ((buf[6 + sec_byte_len] & (0xFF00>>sec_rem_bits))<<24) +
> +            ((unsigned)(s->crc<<16) >> sec_rem_bits));
>

If s->crc is a uint32_t then you don't need to cast it here.
Also argument alignment seems wrong, we align each new line of a function
call/macro with the starting bracket rather than just 4 spaces.
And some spaces in between the shift operators would be nice.


> +        crc_val = av_crc(crc_tab, crc_val, tmp_buf, 3);
> +
> +        if (crc_val) {
> +            av_log(s->avctx, AV_LOG_ERROR, "CRC mismatch %X!\n", crc_val);
>

Good idea, printing the difference was very useful when I was debugging,
we should do that everywhere.


How this works was a little confusing at first but makes sense for me,
so apart from those comments patch LGTM, thanks.
Michael Niedermayer Aug. 4, 2020, 4:08 p.m. UTC | #2
On Mon, Aug 03, 2020 at 08:53:41PM +0200, Lynne wrote:
> Aug 3, 2020, 18:31 by michael@niedermayer.cc:
> 
> > This differs from the MPEG specification as the actual real world
> > files do compute their CRC over variable areas and not the fixed
> > ones listed in the specification. This is also the reason for
> > the complexity of this code and the need to perform the CRC
> > check for layer2 in the middle of layer2 decoding.
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/mpegaudiodec_template.c | 61 +++++++++++++++++++++---------
> >  1 file changed, 44 insertions(+), 17 deletions(-)
> >
> > diff --git a/libavcodec/mpegaudiodec_template.c b/libavcodec/mpegaudiodec_template.c
> > index 3d7e3ba4f2..d84e4f1cb6 100644
> > --- a/libavcodec/mpegaudiodec_template.c
> > +++ b/libavcodec/mpegaudiodec_template.c
> > @@ -89,6 +89,7 @@ typedef struct MPADecodeContext {
> >  MPADSPContext mpadsp;
> >  AVFloatDSPContext *fdsp;
> >  AVFrame *frame;
> > +    int crc;
> >
> 
> Make this a uint32_t, its what we always store CRCs as even if they're 8 bits.
> Thought it was a flag on first read.
> 
> 
> 
> >  } MPADecodeContext;
> >  
> >  #define HEADER_SIZE 4
> > @@ -500,12 +501,43 @@ static void imdct12(INTFLOAT *out, SUINTFLOAT *in)
> >  out[11] = in0 + in5;
> >  }
> >  
> > +static int handle_crc(MPADecodeContext *s, int sec_len)
> > +{
> > +    if (s->error_protection && (s->err_recognition & AV_EF_CRCCHECK)) {
> > +        const uint8_t *buf = s->gb.buffer - HEADER_SIZE;
> > +        int sec_byte_len  = sec_len >> 3;
> > +        int sec_rem_bits  = sec_len & 7;
> > +        const AVCRC *crc_tab = av_crc_get_table(AV_CRC_16_ANSI);
> > +        uint8_t tmp_buf[4];
> > +        uint32_t crc_val = av_crc(crc_tab, UINT16_MAX, &buf[2], 2);
> > +        crc_val = av_crc(crc_tab, crc_val, &buf[6], sec_byte_len);
> > +
> > +        AV_WB32(tmp_buf,
> > +            ((buf[6 + sec_byte_len] & (0xFF00>>sec_rem_bits))<<24) +
> > +            ((unsigned)(s->crc<<16) >> sec_rem_bits));
> >
> 
> If s->crc is a uint32_t then you don't need to cast it here.
> Also argument alignment seems wrong, we align each new line of a function
> call/macro with the starting bracket rather than just 4 spaces.
> And some spaces in between the shift operators would be nice.
> 
> 
> > +        crc_val = av_crc(crc_tab, crc_val, tmp_buf, 3);
> > +
> > +        if (crc_val) {
> > +            av_log(s->avctx, AV_LOG_ERROR, "CRC mismatch %X!\n", crc_val);
> >
> 
> Good idea, printing the difference was very useful when I was debugging,
> we should do that everywhere.
> 
> 
> How this works was a little confusing at first but makes sense for me,
> so apart from those comments patch LGTM, thanks.

ok, will apply with the suggested changes

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/mpegaudiodec_template.c b/libavcodec/mpegaudiodec_template.c
index 3d7e3ba4f2..d84e4f1cb6 100644
--- a/libavcodec/mpegaudiodec_template.c
+++ b/libavcodec/mpegaudiodec_template.c
@@ -89,6 +89,7 @@  typedef struct MPADecodeContext {
     MPADSPContext mpadsp;
     AVFloatDSPContext *fdsp;
     AVFrame *frame;
+    int crc;
 } MPADecodeContext;
 
 #define HEADER_SIZE 4
@@ -500,12 +501,43 @@  static void imdct12(INTFLOAT *out, SUINTFLOAT *in)
     out[11] = in0 + in5;
 }
 
+static int handle_crc(MPADecodeContext *s, int sec_len)
+{
+    if (s->error_protection && (s->err_recognition & AV_EF_CRCCHECK)) {
+        const uint8_t *buf = s->gb.buffer - HEADER_SIZE;
+        int sec_byte_len  = sec_len >> 3;
+        int sec_rem_bits  = sec_len & 7;
+        const AVCRC *crc_tab = av_crc_get_table(AV_CRC_16_ANSI);
+        uint8_t tmp_buf[4];
+        uint32_t crc_val = av_crc(crc_tab, UINT16_MAX, &buf[2], 2);
+        crc_val = av_crc(crc_tab, crc_val, &buf[6], sec_byte_len);
+
+        AV_WB32(tmp_buf,
+            ((buf[6 + sec_byte_len] & (0xFF00>>sec_rem_bits))<<24) +
+            ((unsigned)(s->crc<<16) >> sec_rem_bits));
+
+        crc_val = av_crc(crc_tab, crc_val, tmp_buf, 3);
+
+        if (crc_val) {
+            av_log(s->avctx, AV_LOG_ERROR, "CRC mismatch %X!\n", crc_val);
+            if (s->err_recognition & AV_EF_EXPLODE)
+                return AVERROR_INVALIDDATA;
+        }
+    }
+    return 0;
+}
+
 /* return the number of decoded frames */
 static int mp_decode_layer1(MPADecodeContext *s)
 {
     int bound, i, v, n, ch, j, mant;
     uint8_t allocation[MPA_MAX_CHANNELS][SBLIMIT];
     uint8_t scale_factors[MPA_MAX_CHANNELS][SBLIMIT];
+    int ret;
+
+    ret = handle_crc(s, (s->nb_channels == 1) ? 8*16  : 8*32);
+    if (ret < 0)
+        return ret;
 
     if (s->mode == MPA_JSTEREO)
         bound = (s->mode_ext + 1) * 4;
@@ -575,6 +607,7 @@  static int mp_decode_layer2(MPADecodeContext *s)
     unsigned char scale_code[MPA_MAX_CHANNELS][SBLIMIT];
     unsigned char scale_factors[MPA_MAX_CHANNELS][SBLIMIT][3], *sf;
     int scale, qindex, bits, steps, k, l, m, b;
+    int ret;
 
     /* select decoding table */
     table = ff_mpa_l2_select_table(s->bit_rate / 1000, s->nb_channels,
@@ -617,6 +650,10 @@  static int mp_decode_layer2(MPADecodeContext *s)
         }
     }
 
+    ret = handle_crc(s, get_bits_count(&s->gb) - 16);
+    if (ret < 0)
+        return ret;
+
     /* scale factors */
     for (i = 0; i < sblimit; i++) {
         for (ch = 0; ch < s->nb_channels; ch++) {
@@ -1310,13 +1347,16 @@  static int mp_decode_layer3(MPADecodeContext *s)
     int gr, ch, blocksplit_flag, i, j, k, n, bits_pos;
     GranuleDef *g;
     int16_t exponents[576]; //FIXME try INTFLOAT
+    int ret;
 
     /* read side info */
     if (s->lsf) {
+        ret = handle_crc(s, ((s->nb_channels == 1) ? 8*9  : 8*17));
         main_data_begin = get_bits(&s->gb, 8);
         skip_bits(&s->gb, s->nb_channels);
         nb_granules = 1;
     } else {
+        ret = handle_crc(s, ((s->nb_channels == 1) ? 8*17 : 8*32));
         main_data_begin = get_bits(&s->gb, 9);
         if (s->nb_channels == 2)
             skip_bits(&s->gb, 3);
@@ -1328,6 +1368,8 @@  static int mp_decode_layer3(MPADecodeContext *s)
             s->granules[ch][1].scfsi = get_bits(&s->gb, 4);
         }
     }
+    if (ret < 0)
+        return ret;
 
     for (gr = 0; gr < nb_granules; gr++) {
         for (ch = 0; ch < s->nb_channels; ch++) {
@@ -1565,23 +1607,8 @@  static int mp_decode_frame(MPADecodeContext *s, OUT_INT **samples,
     OUT_INT *samples_ptr;
 
     init_get_bits(&s->gb, buf + HEADER_SIZE, (buf_size - HEADER_SIZE) * 8);
-
-    if (s->error_protection) {
-        uint16_t crc = get_bits(&s->gb, 16);
-        if (s->err_recognition & AV_EF_CRCCHECK) {
-            const int sec_len = s->lsf ? ((s->nb_channels == 1) ? 9  : 17) :
-                                         ((s->nb_channels == 1) ? 17 : 32);
-            const AVCRC *crc_tab = av_crc_get_table(AV_CRC_16_ANSI);
-            uint32_t crc_cal = av_crc(crc_tab, UINT16_MAX, &buf[2], 2);
-            crc_cal = av_crc(crc_tab, crc_cal, &buf[6], sec_len);
-
-            if (av_bswap16(crc) ^ crc_cal) {
-                av_log(s->avctx, AV_LOG_ERROR, "CRC mismatch!\n");
-                if (s->err_recognition & AV_EF_EXPLODE)
-                    return AVERROR_INVALIDDATA;
-            }
-        }
-    }
+    if (s->error_protection)
+        s->crc = get_bits(&s->gb, 16);
 
     switch(s->layer) {
     case 1: