[FFmpeg-devel] [PATCH] avcodec/h264_refs: reset MMCO when invalid mmco code is found

[Paul B Mahol]

On 12/11/18, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> 2018-12-10 23:41 GMT+01:00, Mark Thompson <sw at jkqxz.net>:
>> On 09/12/2018 14:21, Mark Thompson wrote:
>>> On 09/12/2018 13:54, Paul B Mahol wrote:
>>>> On 12/9/18, Mark Thompson <sw at jkqxz.net> wrote:
>>>>> On 09/12/2018 08:52, Paul B Mahol wrote:
>>>>>> On 12/7/18, Paul B Mahol <onemda at gmail.com> wrote:
>>>>>>> On 12/7/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>>> On Fri, Dec 07, 2018 at 10:36:23AM +0100, Paul B Mahol wrote:
>>>>>>>>> On 12/7/18, Paul B Mahol <onemda at gmail.com> wrote:
>>>>>>>>>> On 12/7/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>>>>>>> On Thu, Dec 06, 2018 at 03:26:41PM +0100, Paul B Mahol wrote:
>>>>>>>>>>>> This recovers state with #7374 linked sample.
>>>>>>>>>>>>
>>>>>>>>>>>> Work funded by Open Broadcast Systems.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  libavcodec/h264_refs.c | 2 +-
>>>>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
>>>>>>>>>>>> index eaf965e43d..5645a203a7 100644
>>>>>>>>>>>> --- a/libavcodec/h264_refs.c
>>>>>>>>>>>> +++ b/libavcodec/h264_refs.c
>>>>>>>>>>>> @@ -718,6 +718,7 @@ int
>>>>>>>>>>>> ff_h264_execute_ref_pic_marking(H264Context
>>>>>>>>>>>> *h)
>>>>>>>>>>>>              }
>>>>>>>>>>>>              break;
>>>>>>>>>>>>          case MMCO_RESET:
>>>>>>>>>>>> +        default:
>>>>>>>>>>>>              while (h->short_ref_count) {
>>>>>>>>>>>>                  remove_short(h, h->short_ref[0]->frame_num, 0);
>>>>>>>>>>>>              }
>>>>>>>>>>>> @@ -730,7 +731,6 @@ int
>>>>>>>>>>>> ff_h264_execute_ref_pic_marking(H264Context
>>>>>>>>>>>> *h)
>>>>>>>>>>>>              for (j = 0; j < MAX_DELAYED_PIC_COUNT; j++)
>>>>>>>>>>>>                  h->last_pocs[j] = INT_MIN;
>>>>>>>>>>>>              break;
>>>>>>>>>>>> -        default: assert(0);
>>>>>>>>>>>>          }
>>>>>>>>>>>>      }
>>>>>>>>>>>
>>>>>>>>>>> mmco[i].opcode should not be invalid, its checked around the
>>>>>>>>>>> point
>>>>>>>>>>> where
>>>>>>>>>>> this
>>>>>>>>>>> array is filled.
>>>>>>>>>>> unless there is something iam missing
>>>>>>>>>>
>>>>>>>>>> Yes, you are missing big time.
>>>>>>>>>> If you think by "checked" about those nice asserts they are not
>>>>>>>>>> enabled
>>>>>>>>>> at
>>>>>>>>>> all.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> There is check for invalid opcode, but stored invalid opcode is not
>>>>>>>>> changed.
>>>>>>>>
>>>>>>>> Theres no question that we end with a invalid value in the struct,
>>>>>>>> but
>>>>>>>> that
>>>>>>>> is not intended to be in there. You can see that this is not
>>>>>>>> intended
>>>>>>>> by
>>>>>>>> simply looking at the variable that holds the number of entries, it
>>>>>>>> is
>>>>>>>> not written at all in this case.
>>>>>>>>
>>>>>>>> So for example if this code stores 5 correct looking mmcos and the
>>>>>>>> 6th
>>>>>>>> is
>>>>>>>> invalid, 6 are in the array but the number of entries is just left
>>>>>>>> where
>>>>>>>> it
>>>>>>>> was, that could be maybe 3 or 8 or 1. Just "defaulting out" the
>>>>>>>> invalid
>>>>>>>> value
>>>>>>>> later doesnt feel ideal.
>>>>>>>
>>>>>>> Nope, mmco state is left in inconsistent state, and my patch fix it.
>>>>>>> As
>>>>>>> you
>>>>>>> provided nothing valuable to consider as alternative I will apply it.
>>>>>>>
>>>>>>
>>>>>> What about converting any invalid mmco opcode to mmco reset and
>>>>>> updating mmco size too?
>>>>>> Currently mmco size is left in previous state.
>>>>>
>>>>> Don't invent a new RESET (= 5) operation - that type is special and its
>>>>> presence implies other constraints which we probably don't want to
>>>>> think
>>>>> about here (around frame_num in particular).
>>>>>
>>>>> I think END / truncating the list would be best option?
>>>>>
>>>>
>>>> Nope, that would still put it into bad state. With your approach decoder
>>>> does
>>>> not recover from artifacts. Try sample from bug report #7374.
>>>
>>> Adding a spurious reset here throws away all previous reference frames,
>>> which will break the stream where it wouldn't otherwise be if the
>>> corrupted frame would have been bypassed for referencing.  For example,
>>> in
>>> real-time cases with feedback a stream which has encountered errors can
>>> be
>>> recovered without an intra frame by referring to an older frame which
>>> still exists in the DPB.
>>
>> Sample: <http://ixia.jkqxz.net/~mrt/ffmpeg/no-intra.264>.  The bad frame
>> here has frame_num 24, but we already hit an error before that point and
>> the
>> feedback about that makes frame_num 25 refer to LTRF 1 such that 24 is
>> never
>> used.  (From a simulator rather than a real capture, because random bit
>> errors are never where you want them.)
>>
>> It doesn't exactly hit the original issue because the leftover MMCO count
>> from the previous slice is not large enough.  With:
>>
>> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
>> index 5645a203a7..977b4ed12f 100644
>> --- a/libavcodec/h264_refs.c
>> +++ b/libavcodec/h264_refs.c
>> @@ -875,6 +875,7 @@ int ff_h264_decode_ref_pic_marking(H264SliceContext
>> *sl,
>> GetBitContext *gb,
>>                      av_log(logctx, AV_LOG_ERROR,
>>                             "illegal memory management control operation
>> %d\n",
>>                             opcode);
>> +                    sl->nb_mmco = i + 1;
>>                      return -1;
>>                  }
>>                  if (opcode == MMCO_END)
>>
>> to make sure the MMCO count is written with a high enough value it does.
>> The decoder then loses sync after the inserted reset when that is present
>> and so all frames are wrong, while without the reset sync is maintained
>> and
>> all errors are fixed within a few round trips.
>
> Your change doesn't fix the issue in question...

sl->nb_mmco cant be set to i + 1, as that would still pass invalid value later,
you probably meant to set it to i.