From patchwork Thu Sep 22 09:51:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Claudio Freire X-Patchwork-Id: 676 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.140.66 with SMTP id o63csp1330287vsd; Thu, 22 Sep 2016 02:58:40 -0700 (PDT) X-Received: by 10.28.197.138 with SMTP id v132mr1567714wmf.100.1474538320851; Thu, 22 Sep 2016 02:58:40 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id m135si1494351wmg.47.2016.09.22.02.58.36; Thu, 22 Sep 2016 02:58:40 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@gmail.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2261F6897AC; Thu, 22 Sep 2016 12:58:18 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-it0-f41.google.com (mail-it0-f41.google.com [209.85.214.41]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 2E116687EB7 for ; Thu, 22 Sep 2016 12:58:12 +0300 (EEST) Received: by mail-it0-f41.google.com with SMTP id n143so76417962ita.1 for ; Thu, 22 Sep 2016 02:58:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=i7r9A2go4UHBBUtPBlMfCisQUVnjUsvQMjt/NL/kWBI=; b=KFVWLzycenkNOtlBUgNRlacXLk2uT5WZLmeyxlzqhmKDKuJ9V65+wijMymYC9LfuNM Pp6viTMdn04G+eHbc7e1xYYELqgJOb/jR/sq6zyXi2qm81TgEeVBCBHk96blLdCYAM+K tI7ximF3okxdISx6kNSQNDuaYmDLtrxzNEcwu/htPiKYjlfcApL4+sUtDuvPcUVxSJfY Iltfcu1qxUnmVQlMQQM64Wb+KQ5TVN+BJ/jg+KAGO3OnMYo05ctf4hlDF+QjEkwQXxmG yj9G/6M1++1MJde9Bc7xvekb5aQ2M1x8ypYnutXx591zHBIyQLLoxZ5WYiS7I7K1j9KY tffA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=i7r9A2go4UHBBUtPBlMfCisQUVnjUsvQMjt/NL/kWBI=; b=B7+2zxIASzJaxo10n4ZQ9WEGQej3fhrFXAc9ZUj3fjNoYyGCXeygAQpxAI+RisA30z lyvrKK04C3KCPrBI/a9tZsPOAH9JU/YSmBwzg+4Lm+safTVW+neFYF4lvBWmOGXp0LAm B47JWw/rbPfegm5ADBQuwsSG2t/TJph3X0bG36he4BR3FXLTyaBjK/6/nn742O2quD5J LXZrgRAvscqvCGZTvZ5Tj4HNVTHmRclig55zhOBewoDB3YAb4naEEu9LZcmj91TjZHEY Tj3UTbMlI/aHPzamlBpTaiANcHnMEw1uhJvKdKg6sw4LhR0+t0z1ZwiXFn1WafgIpkiW YhQA== X-Gm-Message-State: AE9vXwPqiOJnCHJL3vSYKqaB8uVWNK8fkL2MKQiQpQjC9yh9JFpCopDh40nmdrEMJR1/QOg6j0tXeOL7PYAGpA== X-Received: by 10.36.53.14 with SMTP id k14mr9334221ita.58.1474537864664; Thu, 22 Sep 2016 02:51:04 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.15.67 with HTTP; Thu, 22 Sep 2016 02:51:03 -0700 (PDT) In-Reply-To: References: <20160823102757.26622-1-michael@niedermayer.cc> From: Claudio Freire Date: Thu, 22 Sep 2016 06:51:03 -0300 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH] avcodec/aaccoder: Limit sf_idx difference for all cases X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On Sat, Sep 10, 2016 at 3:37 AM, Claudio Freire wrote: > On Thu, Aug 25, 2016 at 8:57 AM, Rostislav Pehlivanov > wrote: >>> 64ed96a710787ba5d0666746a8562e7d.dee >>> >>> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind >>> Signed-off-by: Michael Niedermayer >>> --- >>> 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. 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. From 5a3fb7e7fcd156108cc05fb8221722544631c1b7 Mon Sep 17 00:00:00 2001 From: Claudio Freire Date: Thu, 22 Sep 2016 06:37:54 -0300 Subject: [PATCH 4/4] AAC Encoder: prevent TNS from inducing sfdelta asserts TNS filtering can change the required minimum coding book for bands it applies to, so recompute band_type when TNS is being applied. Similarly, zero flags must match for all windows in a window group for code post-tns to work correctly, so make sure twoloop fills in the zero flags appropriately. --- libavcodec/aaccoder.c | 4 ++-- libavcodec/aaccoder_twoloop.h | 14 +++++++++++++- libavcodec/aacenc_tns.c | 21 ++++++++++++++++++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c index 1445ab4..a6edfa4 100644 --- a/libavcodec/aaccoder.c +++ b/libavcodec/aaccoder.c @@ -659,8 +659,8 @@ static void search_for_pns(AACEncContext *s, AVCodecContext *avctx, SingleChanne dist1 += quantize_band_cost(s, &sce->coeffs[start_c], NOR34, sce->ics.swb_sizes[g], - sce->sf_idx[(w+w2)*16+g], - sce->band_alt[(w+w2)*16+g], + sce->sf_idx[w*16+g], + sce->band_alt[w*16+g], lambda/band->threshold, INFINITY, NULL, NULL, 0); /* Estimate rd on average as 5 bits for SF, 4 for the CB, plus spread energy * lambda/thr */ dist2 += band->energy/(band->spread*band->spread)*lambda*dist_thresh/band->threshold; diff --git a/libavcodec/aaccoder_twoloop.h b/libavcodec/aaccoder_twoloop.h index 2098318..1bc1298 100644 --- a/libavcodec/aaccoder_twoloop.h +++ b/libavcodec/aaccoder_twoloop.h @@ -714,7 +714,7 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx, prev = prevsf; sce->sf_idx[w*16+g] = av_clip(sce->sf_idx[w*16+g], prev - SCALE_MAX_DIFF, prev + SCALE_MAX_DIFF); sce->band_type[w*16+g] = find_min_book(maxvals[w*16+g], sce->sf_idx[w*16+g]); - if (sce->band_type[w*16+g]) + if (sce->band_type[w*16+g] == 0) sce->band_type[w*16+g] = 1; prev = sce->sf_idx[w*16+g]; if (!fflag && prevsf != sce->sf_idx[w*16+g]) @@ -760,6 +760,18 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx, } } } + + /** Broadcast zero flags on all windows in the group to keep things consistent */ + for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { + if (sce->ics.group_len[w] > 1) { + /** Make sure proper codebooks are set */ + for (g = 0; g < sce->ics.num_swb; g++) { + int z = sce->zeroes[w*16+g]; + for (w2 = 0; w2 < sce->ics.group_len[w]; w2++) + sce->zeroes[(w+w2)*16+g] = z; + } + } + } } #endif /* AVCODEC_AACCODER_TWOLOOP_H */ diff --git a/libavcodec/aacenc_tns.c b/libavcodec/aacenc_tns.c index 2ffe1f8..2ba1940 100644 --- a/libavcodec/aacenc_tns.c +++ b/libavcodec/aacenc_tns.c @@ -103,9 +103,10 @@ void ff_aac_apply_tns(AACEncContext *s, SingleChannelElement *sce) { TemporalNoiseShaping *tns = &sce->tns; IndividualChannelStream *ics = &sce->ics; - int w, filt, m, i, top, order, bottom, start, end, size, inc; + int w, filt, m, i, top, order, bottom, start, end, size, inc, g; const int mmm = FFMIN(ics->tns_max_bands, ics->max_sfb); float lpc[TNS_MAX_ORDER]; + int has_filt = 0; for (w = 0; w < ics->num_windows; w++) { bottom = ics->num_swb; @@ -137,6 +138,24 @@ void ff_aac_apply_tns(AACEncContext *s, SingleChannelElement *sce) sce->coeffs[start] += lpc[i-1]*sce->pcoeffs[start - i*inc]; } } + + has_filt = 1; + } + } + + if (has_filt) { + abs_pow34_v(s->scoefs, sce->coeffs, 1024); + for (w = 0; w < sce->ics.num_windows; w += sce->ics.group_len[w]) { + start = w*128; + for (g = 0; g < sce->ics.num_swb; g++) { + if (!sce->zeroes[w*16+g]) { + float maxval = find_max_val(sce->ics.group_len[w], sce->ics.swb_sizes[g], s->scoefs+start); + int cb = find_min_book(maxval, sce->sf_idx[w*16+g]); + if (sce->band_type[w*16+g] < cb) + sce->band_type[w*16+g] = cb; + } + start += sce->ics.swb_sizes[g]; + } } } } -- 1.8.4.5