[FFmpeg-devel] [PATCH] avcodec/aaccoder: Limit sf_idx difference for all cases

[Michael Niedermayer]

On Thu, Aug 25, 2016 at 12:57:17PM +0100, Rostislav Pehlivanov wrote:
> On 23 August 2016 at 11:27, Michael Niedermayer <michael at 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 at 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 at 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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160825/2b98e1e9/attachment.sig>