[FFmpeg-devel] [PATCH] mpegaudiodec_template: add ability to check CRC

Michael Niedermayer michael at niedermayer.cc
Thu Mar 14 11:22:41 EET 2019


On Fri, Mar 08, 2019 at 06:48:15PM +0100, Lynne wrote:
> 
> 
> 
> 7 Mar 2019, 21:18 by michael at niedermayer.cc:
> 
> > On Wed, Mar 06, 2019 at 07:09:52PM +0100, Lynne wrote:
> >
> >> A lot of files have CRC included.
> >> The CRC only covers 34 bytes at most from the frame but it should still be
> >> enough for some amount of error detection.
> >>
> >> mpegaudiodec_template.c |   20 +++++++++++++++++---
> >>  1 file changed, 17 insertions(+), 3 deletions(-)
> >> e8276f62fa92aa3f78e53b182b4ca7a2a460754c  0001-mpegaudiodec_template-add-ability-to-check-CRC.patch
> >> From e1f4410f35d3d7f774a0de59ab72764033d14900 Mon Sep 17 00:00:00 2001
> >> From: Lynne <>> dev at lynne.ee <mailto:dev at lynne.ee>>> >
> >> Date: Wed, 6 Mar 2019 17:04:04 +0000
> >> Subject: [PATCH] mpegaudiodec_template: add ability to check CRC
> >>
> >> A lot of files have CRC included.
> >> The CRC only covers 34 bytes at most from the frame but it should still be
> >> enough for some amount of error detection.
> >> ---
> >>  libavcodec/mpegaudiodec_template.c | 20 +++++++++++++++++---
> >>  1 file changed, 17 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/libavcodec/mpegaudiodec_template.c b/libavcodec/mpegaudiodec_template.c
> >> index 9cce88e263..0881b60bf5 100644
> >> --- a/libavcodec/mpegaudiodec_template.c
> >> +++ b/libavcodec/mpegaudiodec_template.c
> >> @@ -27,6 +27,7 @@
> >>  #include "libavutil/attributes.h"
> >>  #include "libavutil/avassert.h"
> >>  #include "libavutil/channel_layout.h"
> >> +#include "libavutil/crc.h"
> >>  #include "libavutil/float_dsp.h"
> >>  #include "libavutil/libm.h"
> >>  #include "avcodec.h"
> >> @@ -1565,9 +1566,22 @@ static int mp_decode_frame(MPADecodeContext *s, OUT_INT **samples,
> >>  
> >>  init_get_bits(&s->gb, buf + HEADER_SIZE, (buf_size - HEADER_SIZE) * 8);
> >>  
> >> -    /* skip error protection field */
> >> -    if (s->error_protection)
> >> -        skip_bits(&s->gb, 16);
> >> +    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;
> >> +            }
> >> +        }
> >> +    }
> >>
> >
> > For files with crcs and with damage, do they sound better with the
> > check and error out or without ?
> >
> > The behavior which provides the best user experience should be the
> > default
> >
> > It also may make sense to add one of AV_EF_CAREFUL / AV_EF_COMPLIANT / AV_EF_AGGRESSIVE
> > depending on how the check affects actual real world files
> >
> 

> This is just a quick check to verify the files are uncorrupted, it shouldn't

As this CRC does not cover the whole data it cannot achieve that


> make broken files sound better unless the decoder somehow outputs white noise
> when it tries to decode them.
> I corrupted some files with -bsf:a noise=0.5 and couldn't notice an improvement
> with or without -err_detect crccheck+explode even though the outputs looked 
> different. crccheck+explode makes much less errors about incorrect timestamps.

If the check doesnt integrate into error concealment which improves subjective
quality (and also cannot really detect corruption reliably) then it is not
that usefull.
There are no doubt situations outside that for which it could be usefull
like detecting errors in parts that would change to sample rate and such 
otherwise but this check is also not designed to protect that codepath.
(libavcodec/mpegaudio_parser.c)

So i think while checking this crc can be usefull, the implementation here
is missing every usecase a bit at least the ones i can see, which no doubt
are not all usecases ...

Thanks


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190314/056f38d9/attachment.sig>


More information about the ffmpeg-devel mailing list