| Message ID | L_JBcUc--3-1@lynne.ee |
|---|---|
| State | New |
| Headers | show |
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
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 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.
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
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
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 [...]
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 >
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 [...]
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.
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)?
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 [...]
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