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

[Marton Balint]

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