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

[Michael Niedermayer]

On Thu, Mar 07, 2019 at 06:32:00PM -0300, James Almer wrote:
> On 3/7/2019 6:18 PM, Michael Niedermayer wrote:
> > 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>
> >> 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 ?
> 
> It depends on the amount of damage. Even a single bit would trigger a
> crc mismatch, but be hardly noticeable.

That is misleading
First, in a compressed stream a single bit especially when part of
some header tends to have quite destructive effects on what
follows. Because things downstream depend on the previous stuff in general
I would have to check how much this applies in this case here but it probably
does. I doubt the CRC would be there if it was just protecting bits you can
nilly willy flip and still decode almost ok.

Also single bit errors are probably rather the exception than the rule.
Everything these days has complex error correction, which would never
pass a single bit error either no errors or lots of errors.

Thus its really a question how real damaged files behave here
They could sound better, in which case assuming the testset was representative
that is better.
They could sound worse in which case i think we should look at our error
handling, to make sure it is working correctly and properly concealing detected
errors. And if its all correct then its better not to error out by default
though maybe the extra information can still be usefull to improve the produced
audio.
The 3rd option would be that there are files out there that are undamaged but
have mismatching CRCs this too would be a good reason not to enable this
be default.
In any case real world files are what matter

Also to return to the single bit errors, you can likely correct them with
the CRC if that is against my expectation a common real world scenario




> 
> > 
> > The behavior which provides the best user experience should be the
> > default
> 
> If the user enables err_recognition and explode flags, both of which are
> currently disabled by default, then aborting on crc failure is the
> expected behavior.

yes


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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20190307/94c4d20c/attachment.sig>