| Message ID | 20160823102757.26622-1-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
On 23 August 2016 at 11:27, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: assertion failure > Fixes: 86914558f0a471f038ee1102c02eeb45/signal_sigabrt_7ffff6ae7c37_3051_ > 64ed96a710787ba5d0666746a8562e7d.dee > > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/aaccoder.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c > index 284b401..995724b 100644 > --- a/libavcodec/aaccoder.c > +++ b/libavcodec/aaccoder.c > @@ -196,7 +196,7 @@ typedef struct TrellisPath { > static void set_special_band_scalefactors(AACEncContext *s, > SingleChannelElement *sce) > { > int w, g; > - int prevscaler_n = -255, prevscaler_i = 0; > + int prevscaler_n = -255, prevscaler_i = 0, prevscaler_d = -255; > int bands = 0; > > for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { > @@ -211,6 +211,10 @@ static void set_special_band_scalefactors(AACEncContext > *s, SingleChannelElement > if (prevscaler_n == -255) > prevscaler_n = sce->sf_idx[w*16+g]; > bands++; > + } else { > + if (prevscaler_d == -255) > + prevscaler_d = sce->sf_idx[w*16+g]; > + bands++; > } > } > } > @@ -227,6 +231,8 @@ static void set_special_band_scalefactors(AACEncContext > *s, SingleChannelElement > sce->sf_idx[w*16+g] = prevscaler_i = > av_clip(sce->sf_idx[w*16+g], prevscaler_i - SCALE_MAX_DIFF, prevscaler_i + > SCALE_MAX_DIFF); > } else if (sce->band_type[w*16+g] == NOISE_BT) { > sce->sf_idx[w*16+g] = prevscaler_n = > av_clip(sce->sf_idx[w*16+g], prevscaler_n - SCALE_MAX_DIFF, prevscaler_n + > SCALE_MAX_DIFF); > + } else { > + sce->sf_idx[w*16+g] = prevscaler_d = > av_clip(sce->sf_idx[w*16+g], prevscaler_d - SCALE_MAX_DIFF, prevscaler_d + > SCALE_MAX_DIFF); > } > } > } > -- > 2.9.3 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > That fuzzed sample seems to be causing the algorithm which does SF difference normalization between normal and PNS bands to fail. This commit masks the problem downstream. IMO that's not the correct way to solve this, as there's no guarantee that another sample won't trigger the same assert even when limiting all scalefactors. Fixing a single fuzzed sample with a hack which doesn't stop other fuzzed samples from triggering the same bug isn't justified. I have the time right now and I'll try to fix this properly, but it might take me a day or two. I think the problem is that when the twoloop coder does the the normalization it doesn't take into account the fact that IS and PNS have their scalefactors modified by set_special_band_scalefactors() later on before encoding.
On Thu, Aug 25, 2016 at 12:57:17PM +0100, Rostislav Pehlivanov wrote: > On 23 August 2016 at 11:27, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Fixes: assertion failure > > Fixes: 86914558f0a471f038ee1102c02eeb45/signal_sigabrt_7ffff6ae7c37_3051_ > > 64ed96a710787ba5d0666746a8562e7d.dee > > > > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/aaccoder.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c > > index 284b401..995724b 100644 > > --- a/libavcodec/aaccoder.c > > +++ b/libavcodec/aaccoder.c > > @@ -196,7 +196,7 @@ typedef struct TrellisPath { > > static void set_special_band_scalefactors(AACEncContext *s, > > SingleChannelElement *sce) > > { > > int w, g; > > - int prevscaler_n = -255, prevscaler_i = 0; > > + int prevscaler_n = -255, prevscaler_i = 0, prevscaler_d = -255; > > int bands = 0; > > > > for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { > > @@ -211,6 +211,10 @@ static void set_special_band_scalefactors(AACEncContext > > *s, SingleChannelElement > > if (prevscaler_n == -255) > > prevscaler_n = sce->sf_idx[w*16+g]; > > bands++; > > + } else { > > + if (prevscaler_d == -255) > > + prevscaler_d = sce->sf_idx[w*16+g]; > > + bands++; > > } > > } > > } > > @@ -227,6 +231,8 @@ static void set_special_band_scalefactors(AACEncContext > > *s, SingleChannelElement > > sce->sf_idx[w*16+g] = prevscaler_i = > > av_clip(sce->sf_idx[w*16+g], prevscaler_i - SCALE_MAX_DIFF, prevscaler_i + > > SCALE_MAX_DIFF); > > } else if (sce->band_type[w*16+g] == NOISE_BT) { > > sce->sf_idx[w*16+g] = prevscaler_n = > > av_clip(sce->sf_idx[w*16+g], prevscaler_n - SCALE_MAX_DIFF, prevscaler_n + > > SCALE_MAX_DIFF); > > + } else { > > + sce->sf_idx[w*16+g] = prevscaler_d = > > av_clip(sce->sf_idx[w*16+g], prevscaler_d - SCALE_MAX_DIFF, prevscaler_d + > > SCALE_MAX_DIFF); > > } > > } > > } > > -- > > 2.9.3 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > That fuzzed sample seems to be causing the algorithm which does SF > difference normalization between normal and PNS bands to fail. This commit > masks the problem downstream. IMO that's not the correct way to solve this, > as there's no guarantee that another sample won't trigger the same assert > even when limiting all scalefactors. Fixing a single fuzzed sample with a > hack which doesn't stop other fuzzed samples from triggering the same bug > isn't justified. thanks for the analysis, i had already suspected that this is possibly not the correct fix, which is why i posted this patch ... > I have the time right now and I'll try to fix this properly, but it might > take me a day or two. I think the problem is that when the twoloop coder > does the the normalization it doesn't take into account the fact that IS > and PNS have their scalefactors modified by set_special_band_scalefactors() > later on before encoding. ok, ill wait with 3.1.3 Thanks [...]
On Thu, Aug 25, 2016 at 8:57 AM, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: >> 64ed96a710787ba5d0666746a8562e7d.dee >> >> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavcodec/aaccoder.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c >> index 284b401..995724b 100644 >> --- a/libavcodec/aaccoder.c >> +++ b/libavcodec/aaccoder.c >> @@ -196,7 +196,7 @@ typedef struct TrellisPath { >> static void set_special_band_scalefactors(AACEncContext *s, >> SingleChannelElement *sce) >> { >> int w, g; >> - int prevscaler_n = -255, prevscaler_i = 0; >> + int prevscaler_n = -255, prevscaler_i = 0, prevscaler_d = -255; >> int bands = 0; >> >> for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { >> @@ -211,6 +211,10 @@ static void set_special_band_scalefactors(AACEncContext >> *s, SingleChannelElement >> if (prevscaler_n == -255) >> prevscaler_n = sce->sf_idx[w*16+g]; >> bands++; >> + } else { >> + if (prevscaler_d == -255) >> + prevscaler_d = sce->sf_idx[w*16+g]; >> + bands++; >> } >> } >> } >> @@ -227,6 +231,8 @@ static void set_special_band_scalefactors(AACEncContext >> *s, SingleChannelElement >> sce->sf_idx[w*16+g] = prevscaler_i = >> av_clip(sce->sf_idx[w*16+g], prevscaler_i - SCALE_MAX_DIFF, prevscaler_i + >> SCALE_MAX_DIFF); >> } else if (sce->band_type[w*16+g] == NOISE_BT) { >> sce->sf_idx[w*16+g] = prevscaler_n = >> av_clip(sce->sf_idx[w*16+g], prevscaler_n - SCALE_MAX_DIFF, prevscaler_n + >> SCALE_MAX_DIFF); >> + } else { >> + sce->sf_idx[w*16+g] = prevscaler_d = >> av_clip(sce->sf_idx[w*16+g], prevscaler_d - SCALE_MAX_DIFF, prevscaler_d + >> SCALE_MAX_DIFF); >> } >> } >> } >> -- >> 2.9.3 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> > > > That fuzzed sample seems to be causing the algorithm which does SF > difference normalization between normal and PNS bands to fail. This commit > masks the problem downstream. IMO that's not the correct way to solve this, > as there's no guarantee that another sample won't trigger the same assert > even when limiting all scalefactors. Fixing a single fuzzed sample with a > hack which doesn't stop other fuzzed samples from triggering the same bug > isn't justified. > I have the time right now and I'll try to fix this properly, but it might > take me a day or two. I think the problem is that when the twoloop coder > does the the normalization it doesn't take into account the fact that IS > and PNS have their scalefactors modified by set_special_band_scalefactors() > later on before encoding. It seems the root of the issue is that the two stages of PNS don't agree on when they can apply PNS or not. I have a WIP that eliminates the issue by just making the two agree, but I've got unrelated changes so I'll try to distill the patch to the minimum necessary to fix this during the weekend.
diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c index 284b401..995724b 100644 --- a/libavcodec/aaccoder.c +++ b/libavcodec/aaccoder.c @@ -196,7 +196,7 @@ typedef struct TrellisPath { static void set_special_band_scalefactors(AACEncContext *s, SingleChannelElement *sce) { int w, g; - int prevscaler_n = -255, prevscaler_i = 0; + int prevscaler_n = -255, prevscaler_i = 0, prevscaler_d = -255; int bands = 0; for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { @@ -211,6 +211,10 @@ static void set_special_band_scalefactors(AACEncContext *s, SingleChannelElement if (prevscaler_n == -255) prevscaler_n = sce->sf_idx[w*16+g]; bands++; + } else { + if (prevscaler_d == -255) + prevscaler_d = sce->sf_idx[w*16+g]; + bands++; } } } @@ -227,6 +231,8 @@ static void set_special_band_scalefactors(AACEncContext *s, SingleChannelElement sce->sf_idx[w*16+g] = prevscaler_i = av_clip(sce->sf_idx[w*16+g], prevscaler_i - SCALE_MAX_DIFF, prevscaler_i + SCALE_MAX_DIFF); } else if (sce->band_type[w*16+g] == NOISE_BT) { sce->sf_idx[w*16+g] = prevscaler_n = av_clip(sce->sf_idx[w*16+g], prevscaler_n - SCALE_MAX_DIFF, prevscaler_n + SCALE_MAX_DIFF); + } else { + sce->sf_idx[w*16+g] = prevscaler_d = av_clip(sce->sf_idx[w*16+g], prevscaler_d - SCALE_MAX_DIFF, prevscaler_d + SCALE_MAX_DIFF); } } }
Fixes: assertion failure Fixes: 86914558f0a471f038ee1102c02eeb45/signal_sigabrt_7ffff6ae7c37_3051_64ed96a710787ba5d0666746a8562e7d.dee Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/aaccoder.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)