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

[Michael Niedermayer]

On Thu, Sep 22, 2016 at 06:51:03AM -0300, Claudio Freire wrote:
> On Sat, Sep 10, 2016 at 3:37 AM, Claudio Freire <klaussfreire at gmail.com> wrote:
> > On Thu, Aug 25, 2016 at 8:57 AM, Rostislav Pehlivanov
> > <atomnuker at gmail.com> wrote:
> >>> 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.
> >> 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.
> 
> Sorry for the delay, it turned out to be more complex than that.
> 
> There were a few potential violations that I had already identified in
> a WIP patch but they did not apply to the fuzzed sample. That sample
> triggered an interaction with TNS and trellis band type coding that
> resulted in zeroed bands reappearing and thus invalidating all delta
> scalefactor validations.
> 
> The attached patch series fixes most of the delta scalefactor
> violation risks I could find, including that one.
> 
> It hasn't been thoroughly tested for quality regressions/improvements.
> It's possible that it does change quality since it changes key
> decision points that conduce to the violations but also to lots of
> audible artifacts. So I believe it should improve quality, but one
> never knows without proper ABX testing, which I'll be conducting, at
> least in a limited way, in the following days.
> 
> In the meantime, I'm attaching the patch series for review.

ping

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20161117/f3a65d22/attachment.sig>