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 <d...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
