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

[Paul B Mahol]

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.