| Message ID | 20210802185005.21829-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | a2690dcccab4f878319e7af42a8ef64f99c15637 |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/mpeg12dec: report error when picture type is unknown and err_detect is EXPLODE | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: > Also split error message to error and warning. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavcodec/mpeg12dec.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > index 858dca660c..49d865853b 100644 > --- a/libavcodec/mpeg12dec.c > +++ b/libavcodec/mpeg12dec.c > @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) > load_matrix(s, s->chroma_inter_matrix, NULL, 0); > } > > -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > { > MpegEncContext *s = &s1->mpeg_enc_ctx; > > @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); > s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); > if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { > - av_log(s->avctx, AV_LOG_ERROR, > - "Missing picture start code, guessing missing values\n"); > + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); > + if (s->avctx->err_recognition & AV_EF_EXPLODE) > + return AVERROR_INVALIDDATA; > + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); If we are nitpicking then this is not ideal "Missing picture start code" is only an error when AV_EF_EXPLODE is set because it only in that case results in wrong output, when AV_EF_EXPLODE is not set the output from the decoder might be correct si it too should be a warning in that case thx [...]
On Fri, 6 Aug 2021, Michael Niedermayer wrote: > On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: >> Also split error message to error and warning. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavcodec/mpeg12dec.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c >> index 858dca660c..49d865853b 100644 >> --- a/libavcodec/mpeg12dec.c >> +++ b/libavcodec/mpeg12dec.c >> @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) >> load_matrix(s, s->chroma_inter_matrix, NULL, 0); >> } >> >> -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >> +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >> { >> MpegEncContext *s = &s1->mpeg_enc_ctx; >> >> @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >> s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); >> s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); >> if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { >> - av_log(s->avctx, AV_LOG_ERROR, >> - "Missing picture start code, guessing missing values\n"); >> + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); >> + if (s->avctx->err_recognition & AV_EF_EXPLODE) >> + return AVERROR_INVALIDDATA; >> + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); > > If we are nitpicking then this is not ideal > "Missing picture start code" is only an error when AV_EF_EXPLODE is set > because it only in that case results in wrong output, when AV_EF_EXPLODE > is not set the output from the decoder might be correct si it too should > be a warning in that case I understand your logic, but there are arguments for keeping the error message as well. I think in practice, most times, this error is caused by bitstream errors and it _will_ affect decoding. Also there are tons of cases in the code which log an error, then return INVALIDDATA if EXPLODE is used, so why handle it here differently... But let me know if you insisit and I will change it. And yes, it is indeed nitpicking :) Regards, Marton
On Sat, Aug 07, 2021 at 09:21:36AM +0200, Marton Balint wrote: > > > On Fri, 6 Aug 2021, Michael Niedermayer wrote: > > > On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: > > > Also split error message to error and warning. > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > --- > > > libavcodec/mpeg12dec.c | 14 ++++++++++---- > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > > > index 858dca660c..49d865853b 100644 > > > --- a/libavcodec/mpeg12dec.c > > > +++ b/libavcodec/mpeg12dec.c > > > @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) > > > load_matrix(s, s->chroma_inter_matrix, NULL, 0); > > > } > > > > > > -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > { > > > MpegEncContext *s = &s1->mpeg_enc_ctx; > > > > > > @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); > > > s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); > > > if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { > > > - av_log(s->avctx, AV_LOG_ERROR, > > > - "Missing picture start code, guessing missing values\n"); > > > + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); > > > + if (s->avctx->err_recognition & AV_EF_EXPLODE) > > > + return AVERROR_INVALIDDATA; > > > + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); > > > > If we are nitpicking then this is not ideal > > "Missing picture start code" is only an error when AV_EF_EXPLODE is set > > because it only in that case results in wrong output, when AV_EF_EXPLODE > > is not set the output from the decoder might be correct si it too should > > be a warning in that case > > I understand your logic, but there are arguments for keeping the error > message as well. I think in practice, most times, this error is caused by > bitstream errors and it _will_ affect decoding. > Also there are tons of cases > in the code which log an error, then return INVALIDDATA if EXPLODE is used, > so why handle it here differently... do these cases really represent situations where the output might be correct ? for examlple any out of range coefficient cant be correct and a mismatching CRC well it "could" be. I think a big part would be, "do we actually have cases where it is not an error ?" if so i would argue that it should be a warning > > But let me know if you insisit and I will change it. And yes, it is indeed > nitpicking :) i dont insist but thats how the levels are defined /** * Something went wrong and cannot losslessly be recovered. * However, not all future data is affected. */ #define AV_LOG_ERROR 16 /** * Something somehow does not look correct. This may or may not * lead to problems. An example would be the use of '-vstrict -2'. */ #define AV_LOG_WARNING 24 [...]
On Sat, 7 Aug 2021, Michael Niedermayer wrote: > On Sat, Aug 07, 2021 at 09:21:36AM +0200, Marton Balint wrote: >> >> >> On Fri, 6 Aug 2021, Michael Niedermayer wrote: >> >>> On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: >>>> Also split error message to error and warning. >>>> >>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>> --- >>>> libavcodec/mpeg12dec.c | 14 ++++++++++---- >>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c >>>> index 858dca660c..49d865853b 100644 >>>> --- a/libavcodec/mpeg12dec.c >>>> +++ b/libavcodec/mpeg12dec.c >>>> @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) >>>> load_matrix(s, s->chroma_inter_matrix, NULL, 0); >>>> } >>>> >>>> -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>> +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>> { >>>> MpegEncContext *s = &s1->mpeg_enc_ctx; >>>> >>>> @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>> s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); >>>> s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); >>>> if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { >>>> - av_log(s->avctx, AV_LOG_ERROR, >>>> - "Missing picture start code, guessing missing values\n"); >>>> + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); >>>> + if (s->avctx->err_recognition & AV_EF_EXPLODE) >>>> + return AVERROR_INVALIDDATA; >>>> + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); >>> >>> If we are nitpicking then this is not ideal >>> "Missing picture start code" is only an error when AV_EF_EXPLODE is set >>> because it only in that case results in wrong output, when AV_EF_EXPLODE >>> is not set the output from the decoder might be correct si it too should >>> be a warning in that case >> >> I understand your logic, but there are arguments for keeping the error >> message as well. I think in practice, most times, this error is caused by >> bitstream errors and it _will_ affect decoding. > >> Also there are tons of cases >> in the code which log an error, then return INVALIDDATA if EXPLODE is used, >> so why handle it here differently... > > do these cases really represent situations where the output might be correct ? > for examlple any out of range coefficient cant be correct and a mismatching CRC > well it "could" be. > I think a big part would be, "do we actually have cases where it is not an > error ?" if so i would argue that it should be a warning Obviously it is hard to be sure. Among the samples we have, I found a few files affected: ffmpeg-bugs/trac/ticket2705/81391.mpg ffmpeg-bugs/trac/ticket2950/mpeg2_fuzz.mpg ffmpeg-bugs/roundup/issue1872/lol-mpeg2.mp4 ffmpeg-bugs/roundup/issue1218/dvbt-ffmpeg-seg-fault.ts All are heavily corrupted files. I also had a private sample file having this issue, and no other error but the "ignoring extra picture following a frame-picture" warning, and it is also a corrupted bitstream, and causes missed frames when decoding. So a patologcal bitstream can probably be created when the decoding is not affected, but my guess is that for real-world files decoding is always affected, therefore I think an error is more appropriate. Let me know what you prefer. Thanks, Marton > >> >> But let me know if you insisit and I will change it. And yes, it is indeed >> nitpicking :) > > i dont insist but thats how the levels are defined > > /** > * Something went wrong and cannot losslessly be recovered. > * However, not all future data is affected. > */ > #define AV_LOG_ERROR 16 > > /** > * Something somehow does not look correct. This may or may not > * lead to problems. An example would be the use of '-vstrict -2'. > */ > #define AV_LOG_WARNING 24 > > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > "I am not trying to be anyone's saviour, I'm trying to think about the > future and not be sad" - Elon Musk > >
On Sat, Aug 07, 2021 at 08:32:35PM +0200, Marton Balint wrote: > > > On Sat, 7 Aug 2021, Michael Niedermayer wrote: > > > On Sat, Aug 07, 2021 at 09:21:36AM +0200, Marton Balint wrote: > > > > > > > > > On Fri, 6 Aug 2021, Michael Niedermayer wrote: > > > > > > > On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: > > > > > Also split error message to error and warning. > > > > > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu> > > > > > --- > > > > > libavcodec/mpeg12dec.c | 14 ++++++++++---- > > > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c > > > > > index 858dca660c..49d865853b 100644 > > > > > --- a/libavcodec/mpeg12dec.c > > > > > +++ b/libavcodec/mpeg12dec.c > > > > > @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) > > > > > load_matrix(s, s->chroma_inter_matrix, NULL, 0); > > > > > } > > > > > > > > > > -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > > > +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > > > { > > > > > MpegEncContext *s = &s1->mpeg_enc_ctx; > > > > > > > > > > @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) > > > > > s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); > > > > > s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); > > > > > if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { > > > > > - av_log(s->avctx, AV_LOG_ERROR, > > > > > - "Missing picture start code, guessing missing values\n"); > > > > > + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); > > > > > + if (s->avctx->err_recognition & AV_EF_EXPLODE) > > > > > + return AVERROR_INVALIDDATA; > > > > > + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); > > > > > > > > If we are nitpicking then this is not ideal > > > > "Missing picture start code" is only an error when AV_EF_EXPLODE is set > > > > because it only in that case results in wrong output, when AV_EF_EXPLODE > > > > is not set the output from the decoder might be correct si it too should > > > > be a warning in that case > > > > > > I understand your logic, but there are arguments for keeping the error > > > message as well. I think in practice, most times, this error is caused by > > > bitstream errors and it _will_ affect decoding. > > > > > Also there are tons of cases > > > in the code which log an error, then return INVALIDDATA if EXPLODE is used, > > > so why handle it here differently... > > > > do these cases really represent situations where the output might be correct ? > > for examlple any out of range coefficient cant be correct and a mismatching CRC > > well it "could" be. > > I think a big part would be, "do we actually have cases where it is not an > > error ?" if so i would argue that it should be a warning > > Obviously it is hard to be sure. Among the samples we have, I found a few > files affected: > > ffmpeg-bugs/trac/ticket2705/81391.mpg > ffmpeg-bugs/trac/ticket2950/mpeg2_fuzz.mpg > ffmpeg-bugs/roundup/issue1872/lol-mpeg2.mp4 > ffmpeg-bugs/roundup/issue1218/dvbt-ffmpeg-seg-fault.ts > > All are heavily corrupted files. > > I also had a private sample file having this issue, and no other error but > the "ignoring extra picture following a frame-picture" warning, and it is > also a corrupted bitstream, and causes missed frames when decoding. > > So a patologcal bitstream can probably be created when the decoding is not > affected, but my guess is that for real-world files decoding is always > affected, therefore I think an error is more appropriate. i thought i saw one where it was just missing and no corruption but maybe not. Its certainly not worth the time spent on this to search further > > Let me know what you prefer. please use what you prefer thx [...]
On Sun, 8 Aug 2021, Michael Niedermayer wrote: > On Sat, Aug 07, 2021 at 08:32:35PM +0200, Marton Balint wrote: >> >> >> On Sat, 7 Aug 2021, Michael Niedermayer wrote: >> >>> On Sat, Aug 07, 2021 at 09:21:36AM +0200, Marton Balint wrote: >>>> >>>> >>>> On Fri, 6 Aug 2021, Michael Niedermayer wrote: >>>> >>>>> On Mon, Aug 02, 2021 at 08:50:05PM +0200, Marton Balint wrote: >>>>>> Also split error message to error and warning. >>>>>> >>>>>> Signed-off-by: Marton Balint <cus@passwd.hu> >>>>>> --- >>>>>> libavcodec/mpeg12dec.c | 14 ++++++++++---- >>>>>> 1 file changed, 10 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c >>>>>> index 858dca660c..49d865853b 100644 >>>>>> --- a/libavcodec/mpeg12dec.c >>>>>> +++ b/libavcodec/mpeg12dec.c >>>>>> @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) >>>>>> load_matrix(s, s->chroma_inter_matrix, NULL, 0); >>>>>> } >>>>>> >>>>>> -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>>>> +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>>>> { >>>>>> MpegEncContext *s = &s1->mpeg_enc_ctx; >>>>>> >>>>>> @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) >>>>>> s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); >>>>>> s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); >>>>>> if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { >>>>>> - av_log(s->avctx, AV_LOG_ERROR, >>>>>> - "Missing picture start code, guessing missing values\n"); >>>>>> + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); >>>>>> + if (s->avctx->err_recognition & AV_EF_EXPLODE) >>>>>> + return AVERROR_INVALIDDATA; >>>>>> + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); >>>>> >>>>> If we are nitpicking then this is not ideal >>>>> "Missing picture start code" is only an error when AV_EF_EXPLODE is set >>>>> because it only in that case results in wrong output, when AV_EF_EXPLODE >>>>> is not set the output from the decoder might be correct si it too should >>>>> be a warning in that case >>>> >>>> I understand your logic, but there are arguments for keeping the error >>>> message as well. I think in practice, most times, this error is caused by >>>> bitstream errors and it _will_ affect decoding. >>> >>>> Also there are tons of cases >>>> in the code which log an error, then return INVALIDDATA if EXPLODE is used, >>>> so why handle it here differently... >>> >>> do these cases really represent situations where the output might be correct ? >>> for examlple any out of range coefficient cant be correct and a mismatching CRC >>> well it "could" be. >>> I think a big part would be, "do we actually have cases where it is not an >>> error ?" if so i would argue that it should be a warning >> >> Obviously it is hard to be sure. Among the samples we have, I found a few >> files affected: >> >> ffmpeg-bugs/trac/ticket2705/81391.mpg >> ffmpeg-bugs/trac/ticket2950/mpeg2_fuzz.mpg >> ffmpeg-bugs/roundup/issue1872/lol-mpeg2.mp4 >> ffmpeg-bugs/roundup/issue1218/dvbt-ffmpeg-seg-fault.ts >> >> All are heavily corrupted files. >> >> I also had a private sample file having this issue, and no other error but >> the "ignoring extra picture following a frame-picture" warning, and it is >> also a corrupted bitstream, and causes missed frames when decoding. >> > >> So a patologcal bitstream can probably be created when the decoding is not >> affected, but my guess is that for real-world files decoding is always >> affected, therefore I think an error is more appropriate. > > i thought i saw one where it was just missing and no corruption but maybe > not. Its certainly not worth the time spent on this to search further > > >> >> Let me know what you prefer. > > please use what you prefer Ok, applied my version then. Thanks, Marton
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index 858dca660c..49d865853b 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -1529,7 +1529,7 @@ static void mpeg_decode_quant_matrix_extension(MpegEncContext *s) load_matrix(s, s->chroma_inter_matrix, NULL, 0); } -static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) +static int mpeg_decode_picture_coding_extension(Mpeg1Context *s1) { MpegEncContext *s = &s1->mpeg_enc_ctx; @@ -1539,8 +1539,10 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) s->mpeg_f_code[1][0] = get_bits(&s->gb, 4); s->mpeg_f_code[1][1] = get_bits(&s->gb, 4); if (!s->pict_type && s1->mpeg_enc_ctx_allocated) { - av_log(s->avctx, AV_LOG_ERROR, - "Missing picture start code, guessing missing values\n"); + av_log(s->avctx, AV_LOG_ERROR, "Missing picture start code\n"); + if (s->avctx->err_recognition & AV_EF_EXPLODE) + return AVERROR_INVALIDDATA; + av_log(s->avctx, AV_LOG_WARNING, "Guessing pict_type from mpeg_f_code\n"); if (s->mpeg_f_code[1][0] == 15 && s->mpeg_f_code[1][1] == 15) { if (s->mpeg_f_code[0][0] == 15 && s->mpeg_f_code[0][1] == 15) s->pict_type = AV_PICTURE_TYPE_I; @@ -1586,6 +1588,8 @@ static void mpeg_decode_picture_coding_extension(Mpeg1Context *s1) ff_dlog(s->avctx, "alternate_scan=%d\n", s->alternate_scan); ff_dlog(s->avctx, "frame_pred_frame_dct=%d\n", s->frame_pred_frame_dct); ff_dlog(s->avctx, "progressive_frame=%d\n", s->progressive_frame); + + return 0; } static int mpeg_field_start(MpegEncContext *s, const uint8_t *buf, int buf_size) @@ -2599,7 +2603,9 @@ static int decode_chunks(AVCodecContext *avctx, AVFrame *picture, break; case 0x8: if (last_code == PICTURE_START_CODE) { - mpeg_decode_picture_coding_extension(s); + int ret = mpeg_decode_picture_coding_extension(s); + if (ret < 0) + return ret; } else { av_log(avctx, AV_LOG_ERROR, "ignoring pic cod ext after %X\n", last_code);
Also split error message to error and warning. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavcodec/mpeg12dec.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)