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

[Paul B Mahol]

On 12/9/18, Mark Thompson <sw at jkqxz.net> 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.

So instead of fixing all frames after error you prefer keeping old
code which will keep spurious errors all the time, keeping decoder
useless.