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

Submitted by Paul B Mahol on Dec. 6, 2018, 2:26 p.m.

Details

Message ID 20181206142641.3441-1-onemda@gmail.com
State New
Headers show

Commit Message

Paul B Mahol Dec. 6, 2018, 2:26 p.m.
This recovers state with #7374 linked sample.

Work funded by Open Broadcast Systems.

Signed-off-by: Paul B Mahol <onemda@gmail.com>
---
 libavcodec/h264_refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Dec. 7, 2018, 3:04 a.m.
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@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

[...]
Paul B Mahol Dec. 7, 2018, 9:31 a.m.
On 12/7/18, Michael Niedermayer <michael@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@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.
Paul B Mahol Dec. 7, 2018, 9:36 a.m.
On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
> On 12/7/18, Michael Niedermayer <michael@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@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.
Michael Niedermayer Dec. 7, 2018, 4:40 p.m.
On Fri, Dec 07, 2018 at 10:36:23AM +0100, Paul B Mahol wrote:
> On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
> > On 12/7/18, Michael Niedermayer <michael@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@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.


[...]
Paul B Mahol Dec. 7, 2018, 5:15 p.m.
On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>> > On 12/7/18, Michael Niedermayer <michael@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@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.
Paul B Mahol Dec. 9, 2018, 8:52 a.m.
On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
> On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>>> > On 12/7/18, Michael Niedermayer <michael@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@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.
Mark Thompson Dec. 9, 2018, 1:34 p.m.
On 09/12/2018 08:52, Paul B Mahol wrote:
> On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
>> On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>>>>> On 12/7/18, Michael Niedermayer <michael@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@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
Paul B Mahol Dec. 9, 2018, 1:54 p.m.
On 12/9/18, Mark Thompson <sw@jkqxz.net> wrote:
> On 09/12/2018 08:52, Paul B Mahol wrote:
>> On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>>>>>> On 12/7/18, Michael Niedermayer <michael@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@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.
Mark Thompson Dec. 9, 2018, 2:21 p.m.
On 09/12/2018 13:54, Paul B Mahol wrote:
> On 12/9/18, Mark Thompson <sw@jkqxz.net> wrote:
>> On 09/12/2018 08:52, Paul B Mahol wrote:
>>> On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
>>>> On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>>>>>>> On 12/7/18, Michael Niedermayer <michael@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@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.

- Mark
Paul B Mahol Dec. 9, 2018, 4:34 p.m.
On 12/9/18, Mark Thompson <sw@jkqxz.net> wrote:
> On 09/12/2018 13:54, Paul B Mahol wrote:
>> On 12/9/18, Mark Thompson <sw@jkqxz.net> wrote:
>>> On 09/12/2018 08:52, Paul B Mahol wrote:
>>>> On 12/7/18, Paul B Mahol <onemda@gmail.com> wrote:
>>>>> On 12/7/18, Michael Niedermayer <michael@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@gmail.com> wrote:
>>>>>>>> On 12/7/18, Michael Niedermayer <michael@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@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.

Patch hide | download patch | download mbox

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);
         }
     }