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

Submitted by Lynne on March 6, 2019, 6:09 p.m.

Details

Message ID L_JBcUc--3-1@lynne.ee
State New
Headers show

Commit Message

Lynne March 6, 2019, 6:09 p.m.
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.

Comments

Yufei He March 6, 2019, 7:04 p.m.
Hi

I want to use   ff_extract_extradata_bsf to get extradata from a h.264 frame.

Here is the code.

AVPacket *avpkt; // there is valid data.
AVBSFContext *ctx = NULL;
ret = av_bsf_alloc(&ff_extract_extradata_bsf, &ctx);
ret = ff_extract_extradata_bsf.init(ctx);
ret = ff_extract_extradata_bsf.filter(ctx, avpkt);

ff_extract_extradata_bsf.filter failed on calling ff_bsf_get_packet_ref because  ctx->internal->buffer+pkt->data is NULL.

int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
{
AVBSFInternal *in = ctx->internal;
if (in->eof)
return AVERROR_EOF;
if (!ctx->internal->buffer_pkt->data &&
!ctx->internal->buffer_pkt->side_data_elems)
return AVERROR(EAGAIN);

How should ctx->internal->buffer+pkt->data be set?

Thanks.

Yufei.



On 03/06/2019 01:09 PM, 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.




_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org<mailto:ffmpeg-devel@ffmpeg.org>
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Yufei He March 6, 2019, 7:08 p.m.
Hi

I want to use   ff_extract_extradata_bsf to get extradata from a h.264 frame.

Here is the code.

AVPacket *avpkt; // There is valid data.
AVBSFContext *ctx = NULL;
ret = av_bsf_alloc(&ff_extract_extradata_bsf, &ctx);
ret = ff_extract_extradata_bsf.init(ctx);
ret = ff_extract_extradata_bsf.filter(ctx, avpkt);

ff_extract_extradata_bsf.filter failed on calling ff_bsf_get_packet_ref because  ctx->internal->buffer_pkt->data is NULL.

int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
{
AVBSFInternal *in = ctx->internal;
if (in->eof)
return AVERROR_EOF;
if (!ctx->internal->buffer_pkt->data &&
!ctx->internal->buffer_pkt->side_data_elems)
return AVERROR(EAGAIN);

...
}


How should ctx->internal->buffer+pkt->data be set?

Thanks.

Yufei.
James Almer March 6, 2019, 7:10 p.m.
On 3/6/2019 4:04 PM, Yufei He wrote:
> Hi
> 
> I want to use   ff_extract_extradata_bsf to get extradata from a h.264 frame.
> 
> Here is the code.
> 
> AVPacket *avpkt; // there is valid data.
> AVBSFContext *ctx = NULL;
> ret = av_bsf_alloc(&ff_extract_extradata_bsf, &ctx);
> ret = ff_extract_extradata_bsf.init(ctx);
> ret = ff_extract_extradata_bsf.filter(ctx, avpkt);
> 
> ff_extract_extradata_bsf.filter failed on calling ff_bsf_get_packet_ref because  ctx->internal->buffer+pkt->data is NULL.
> 
> int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
> {
> AVBSFInternal *in = ctx->internal;
> if (in->eof)
> return AVERROR_EOF;
> if (!ctx->internal->buffer_pkt->data &&
> !ctx->internal->buffer_pkt->side_data_elems)
> return AVERROR(EAGAIN);
> 
> How should ctx->internal->buffer+pkt->data be set?
> 
> Thanks.
> 
> Yufei.

You're not using the bsf API correctly. You're accessing internal
callbacks like init() and filter() directly. Use the public functions
defined in avcodec.h, and look at usage examples like in ffmpeg.c if needed.

Also, any further questions or emails about this should go to the
libav-user list at https://lists.ffmpeg.org/mailman/listinfo/libav-user/
This list is for actual ffmpeg development, not for usage questions.
Yufei He March 7, 2019, 2:35 p.m.
Hi James

Thanks. I got the extradata from first encoded frame with functions in avcodec.h and examples in ffmpeg.c.

My problem is that the trancoded MP4 file does not have valid AVCC box. It's supposed to have the extradata from the codec.

I can only get the extradata after I receive the first encoded frame, while I don't have any frame in my encoder's init function.

It seems ffmpeg can only generate AVCC box if I set extradata in my encoder's init function, it does not take the extradata if I make it in receive_packet function.

Please help.

Yufei.
if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER && !avctx->extradata) {
const AVBitStreamFilter *filter = av_bsf_get_by_name("extract_extradata");
ret = av_bsf_alloc(filter, &ctx);
if (ret < 0)
return ret;
ret = avcodec_parameters_from_context(ctx->par_in, avctx);
if (ret < 0)
return ret;
ret = av_bsf_init(ctx);
if (ret < 0)
return ret;
ctx->time_base_in = avctx->time_base;
ret = av_bsf_send_packet(ctx, avpkt);
if (ret < 0)
return ret;
ret = av_bsf_receive_packet(ctx, avpkt);
if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
return 0;
if (avpkt->side_data_elems > 0 )
{
avctx->extradata = av_mallocz(avpkt->side_data->size + AV_INPUT_BUFFER_PADDING_SIZE);
if (!avctx->extradata)
return AVERROR(ENOMEM);
memcpy(avctx->extradata, avpkt->side_data->data, avpkt->side_data->size);
avctx->extradata_size = avpkt->side_data->size;
}
av_bsf_free(&ctx);
}

On 03/06/2019 02:10 PM, James Almer wrote:

On 3/6/2019 4:04 PM, Yufei He wrote:


Hi

I want to use   ff_extract_extradata_bsf to get extradata from a h.264 frame.

Here is the code.

AVPacket *avpkt; // there is valid data.
AVBSFContext *ctx = NULL;
ret = av_bsf_alloc(&ff_extract_extradata_bsf, &ctx);
ret = ff_extract_extradata_bsf.init(ctx);
ret = ff_extract_extradata_bsf.filter(ctx, avpkt);

ff_extract_extradata_bsf.filter failed on calling ff_bsf_get_packet_ref because  ctx->internal->buffer+pkt->data is NULL.

int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
{
AVBSFInternal *in = ctx->internal;
if (in->eof)
return AVERROR_EOF;
if (!ctx->internal->buffer_pkt->data &&
!ctx->internal->buffer_pkt->side_data_elems)
return AVERROR(EAGAIN);

How should ctx->internal->buffer+pkt->data be set?

Thanks.

Yufei.



You're not using the bsf API correctly. You're accessing internal
callbacks like init() and filter() directly. Use the public functions
defined in avcodec.h, and look at usage examples like in ffmpeg.c if needed.

Also, any further questions or emails about this should go to the
libav-user list at https://lists.ffmpeg.org/mailman/listinfo/libav-user/
This list is for actual ffmpeg development, not for usage questions.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org<mailto:ffmpeg-devel@ffmpeg.org>
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Yufei He March 7, 2019, 5:38 p.m.
Hi

I have a problem that the trancoded MP4 file does not have valid AVCC box. It's supposed to have the extradata from the codec.

I can only get the extradata after I receive the first encoded frame, while I don't have any frame in my encoder's init function.

It seems ffmpeg can only generate AVCC box if I set extradata in my encoder's init function, it does not take the extradata if I make it in receive_packet function.

Please help.

Thanks.

Yufei.
if (avctx->flags & AV_CODEC_FLAG_GLOBAL_HEADER && !avctx->extradata) {
const AVBitStreamFilter *filter = av_bsf_get_by_name("extract_extradata");
ret = av_bsf_alloc(filter, &ctx);
if (ret < 0)
return ret;
ret = avcodec_parameters_from_context(ctx->par_in, avctx);
if (ret < 0)
return ret;
ret = av_bsf_init(ctx);
if (ret < 0)
return ret;
ctx->time_base_in = avctx->time_base;
ret = av_bsf_send_packet(ctx, avpkt);
if (ret < 0)
return ret;
ret = av_bsf_receive_packet(ctx, avpkt);
if (ret == AVERROR(EAGAIN) || ret == AVERROR_EOF)
return 0;
if (avpkt->side_data_elems > 0 )
{
avctx->extradata = av_mallocz(avpkt->side_data->size + AV_INPUT_BUFFER_PADDING_SIZE);
if (!avctx->extradata)
return AVERROR(ENOMEM);
memcpy(avctx->extradata, avpkt->side_data->data, avpkt->side_data->size);
avctx->extradata_size = avpkt->side_data->size;
}
av_bsf_free(&ctx);
}

On 03/06/2019 02:10 PM, James Almer wrote:

On 3/6/2019 4:04 PM, Yufei He wrote:


Hi

I want to use   ff_extract_extradata_bsf to get extradata from a h.264 frame.

Here is the code.

AVPacket *avpkt; // there is valid data.
AVBSFContext *ctx = NULL;
ret = av_bsf_alloc(&ff_extract_extradata_bsf, &ctx);
ret = ff_extract_extradata_bsf.init(ctx);
ret = ff_extract_extradata_bsf.filter(ctx, avpkt);

ff_extract_extradata_bsf.filter failed on calling ff_bsf_get_packet_ref because  ctx->internal->buffer+pkt->data is NULL.

int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket *pkt)
{
AVBSFInternal *in = ctx->internal;
if (in->eof)
return AVERROR_EOF;
if (!ctx->internal->buffer_pkt->data &&
!ctx->internal->buffer_pkt->side_data_elems)
return AVERROR(EAGAIN);

How should ctx->internal->buffer+pkt->data be set?

Thanks.

Yufei.


You're not using the bsf API correctly. You're accessing internal
callbacks like init() and filter() directly. Use the public functions
defined in avcodec.h, and look at usage examples like in ffmpeg.c if needed.

Also, any further questions or emails about this should go to the
libav-user list at https://lists.ffmpeg.org/mailman/listinfo/libav-user/
This list is for actual ffmpeg development, not for usage questions.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org<mailto:ffmpeg-devel@ffmpeg.org>
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer March 7, 2019, 9:18 p.m.
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@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

thx

[...]
James Almer March 7, 2019, 9:32 p.m.
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@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.

> 
> 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.

> 
> 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
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer March 7, 2019, 10:10 p.m.
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@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


[...]
Lynne March 8, 2019, 5:48 p.m.
7 Mar 2019, 21:18 by michael@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@lynne.ee <mailto:dev@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
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.
Lynne March 13, 2019, 3:54 p.m.
8 Mar 2019, 17:48 by dev@lynne.ee:

>
>
>
> 7 Mar 2019, 21:18 by > michael@niedermayer.cc <mailto:michael@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@lynne.ee <mailto:dev@lynne.ee>>>>  <mailto:>>> dev@lynne.ee <mailto:dev@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
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org <mailto:ffmpeg-devel@ffmpeg.org>
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel <https://ffmpeg.org/mailman/listinfo/ffmpeg-devel>
>

Ping? Could someone take a look at this patch and my other 2 (apedec and pngdec CRC)?
Michael Niedermayer March 14, 2019, 9:22 a.m.
On Fri, Mar 08, 2019 at 06:48:15PM +0100, Lynne wrote:
> 
> 
> 
> 7 Mar 2019, 21:18 by michael@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@lynne.ee <mailto:dev@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


[...]

Patch hide | download patch | download mbox

From e1f4410f35d3d7f774a0de59ab72764033d14900 Mon Sep 17 00:00:00 2001
From: Lynne <dev@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;
+            }
+        }
+    }
 
     switch(s->layer) {
     case 1:
-- 
2.21.0