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

[Michael Niedermayer]

On Mon, Aug 03, 2020 at 08:53:41PM +0200, Lynne wrote:
> Aug 3, 2020, 18:31 by michael at 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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Rewriting code that is poorly written but fully understood is good.
Rewriting code that one doesnt understand is a sign that one is less smart
then the original author, trying to rewrite it will not make it better.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200804/258843d5/attachment.sig>