Aug 3, 2020, 18:31 by mich...@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 <mich...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
