[FFmpeg-devel] [PATCH] avcodec/aacenc_quantization: Fix undefined behavior and instead detect and print an error

[Claudio Freire]

On Wed, Mar 30, 2016 at 1:18 AM, Claudio Freire <klaussfreire at gmail.com> wrote:
> On Tue, Mar 29, 2016 at 10:51 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> This is a hotfix and not a real fix of the underlaying bug
>> The underlaying bug is ATM not fully understood
>>
>> iam not sure if we should apply this or not
>>
>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>> ---
>>  libavcodec/aacenc_quantization.h |   13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/aacenc_quantization.h b/libavcodec/aacenc_quantization.h
>> index 4250407..d367be0 100644
>> --- a/libavcodec/aacenc_quantization.h
>> +++ b/libavcodec/aacenc_quantization.h
>> @@ -141,8 +141,17 @@ static av_always_inline float quantize_and_encode_band_cost_template(
>>              if (BT_ESC) {
>>                  for (j = 0; j < 2; j++) {
>>                      if (ff_aac_codebook_vectors[cb-1][curidx*2+j] == 64.0f) {
>> -                        int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13);
>> -                        int len = av_log2(coef);
>> +                        float a = fabsf(in[i+j]) * Q;
>> +                        double f = sqrtf(a * sqrtf(a)) + ROUNDING;
>> +                        int coef, len;
>> +
>> +                        if (f > INT_MAX || f < 16) {
>> +                            av_log(NULL, AV_LOG_ERROR, "f %f is out of range this is a internal error\n", f);
>> +                            f = INT_MAX;
>> +                        }
>> +
>> +                        coef = av_clip_uintp2(f, 13);
>> +                        len = av_log2(coef);
>>
>>                          put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2);
>>                          put_sbits(pb, len, coef);
>
> Actually I just understood the underlying bug and am testing a fix.
>
> Basically, scalefactors need to be bound by (roughly)
> coef2minsf(maxval), which isn't being done atm, and some signals
> prompt the encoder to pick lower and lower scalefactors trying to
> consume unspent bits that cannot really be consumed.

Try the attached diff instead (I'm not able to reproduce the issue with gcc)
-------------- next part --------------
diff --git a/libavcodec/aaccoder_twoloop.h b/libavcodec/aaccoder_twoloop.h
index 397a4db..4747c79 100644
--- a/libavcodec/aaccoder_twoloop.h
+++ b/libavcodec/aaccoder_twoloop.h
@@ -77,7 +77,7 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx,
     int toomanybits, toofewbits;
     char nzs[128];
     uint8_t nextband[128];
-    int maxsf[128];
+    int maxsf[128], minsf[128];
     float dists[128] = { 0 }, qenergies[128] = { 0 }, uplims[128], euplims[128], energies[128];
     float maxvals[128], spread_thr_r[128];
     float min_spread_thr_r, max_spread_thr_r;
@@ -294,11 +294,14 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx,
     abs_pow34_v(s->scoefs, sce->coeffs, 1024);
     ff_quantize_band_cost_cache_init(s);
 
+    for (i = 0; i < sizeof(minsf) / sizeof(minsf[0]); ++i)
+        minsf[i] = 0;
     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++) {
             const float *scaled = s->scoefs + start;
             maxvals[w*16+g] = find_max_val(sce->ics.group_len[w], sce->ics.swb_sizes[g], scaled);
+            minsf[w*16+g] = coef2minsf(maxvals[w*16+g]);
             start += sce->ics.swb_sizes[g];
         }
     }
@@ -425,7 +428,7 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx,
                 recomprd = 1;
                 for (i = 0; i < 128; i++) {
                     if (sce->sf_idx[i] > SCALE_ONE_POS) {
-                        int new_sf = FFMAX(SCALE_ONE_POS, sce->sf_idx[i] - qstep);
+                        int new_sf = FFMAX3(minsf[i], SCALE_ONE_POS, sce->sf_idx[i] - qstep);
                         if (new_sf != sce->sf_idx[i]) {
                             sce->sf_idx[i] = new_sf;
                             changed = 1;
@@ -595,7 +598,7 @@ static void search_for_quantizers_twoloop(AVCodecContext *avctx,
                     int cmb = find_min_book(maxvals[w*16+g], sce->sf_idx[w*16+g]);
                     int mindeltasf = FFMAX(0, prev - SCALE_MAX_DIFF);
                     int maxdeltasf = FFMIN(SCALE_MAX_POS - SCALE_DIV_512, prev + SCALE_MAX_DIFF);
-                    if ((!cmb || dists[w*16+g] > uplims[w*16+g]) && sce->sf_idx[w*16+g] > mindeltasf) {
+                    if ((!cmb || dists[w*16+g] > uplims[w*16+g]) && sce->sf_idx[w*16+g] > FFMAX(mindeltasf, minsf[w*16+g])) {
                         /* Try to make sure there is some energy in every nonzero band
                          * NOTE: This algorithm must be forcibly imbalanced, pushing harder
                          *  on holes or more distorted bands at first, otherwise there's