[FFmpeg-devel] [PATCH v2] avcodec/mpeg12dec: report error when picture type is unknown and err_detect is EXPLODE

[Marton Balint]

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