On 12/11/18, Carl Eugen Hoyos <ceffm...@gmail.com> wrote: > 2018-12-10 23:41 GMT+01:00, Mark Thompson <s...@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 <s...@jkqxz.net> wrote: >>>>> On 09/12/2018 08:52, Paul B Mahol wrote: >>>>>> On 12/7/18, Paul B Mahol <one...@gmail.com> wrote: >>>>>>> On 12/7/18, Michael Niedermayer <mich...@niedermayer.cc> wrote: >>>>>>>> On Fri, Dec 07, 2018 at 10:36:23AM +0100, Paul B Mahol wrote: >>>>>>>>> On 12/7/18, Paul B Mahol <one...@gmail.com> wrote: >>>>>>>>>> On 12/7/18, Michael Niedermayer <mich...@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 <one...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel