| Message ID | 925308a9-aaf5-a371-622b-1c73c7b508bb@jkqxz.net |
|---|---|
| State | New |
| Headers | show |
2018-12-10 23:41 GMT+01:00, Mark Thompson <sw@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@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. > > 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... Carl Eugen
On 12/11/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-10 23:41 GMT+01:00, Mark Thompson <sw@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@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. >> >> 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.
On 11/12/2018 14:28, Paul B Mahol wrote: > On 12/11/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-12-10 23:41 GMT+01:00, Mark Thompson <sw@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@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. >>> >>> 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
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)