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

[Mark Thompson]

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?

- Mark