[FFmpeg-devel] [PATCH] avcodec/h264_refs: reset MMCO when invalid mmco code is found
Mark Thompson
sw at jkqxz.net
Tue Dec 18 00:03:48 EET 2018
On 11/12/2018 14:28, Paul B Mahol wrote:
> 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.
I think I wasn't clear there - the patch above ensures that the sample always hits the original error case (with the fake assert). It doesn't fix anything; indeed, it will make bad cases worse.
I would be happy with your suggested answer of setting nb_mmco to i in the error paths out of ff_h264_decode_ref_pic_marking (and replacing the fake assert with a real assert, since with that change it really shouldn't be able to happen).
Thanks,
- Mark
More information about the ffmpeg-devel
mailing list