[Ffmpeg-cvslog] r5507 - trunk/libavcodec/flac.c

[Justin Ruggles]

Loren Merritt wrote:
> On Tue, 20 Jun 2006, Justin Ruggles wrote:
> 
>>lu_zero wrote:
>>
>>>-    for (i = pred_order; i < s->blocksize; i++)
>>>-    {
>>>-        sum = 0;
>>>-        for (j = 0; j < pred_order; j++)
>>>-            sum += coeffs[j] * s->decoded[channel][i-j-1];
>>>-        s->decoded[channel][i] += sum >> qlevel;
>>>+    if (s->bps > 16) {
>>>+        int64_t sum;
>>>+        for (i = pred_order; i < s->blocksize; i++)
>>>+        {
>>>+            sum = 0;
>>>+            for (j = 0; j < pred_order; j++)
>>>+                sum += (int64_t)coeffs[j] * s->decoded[channel][i-j-1];
>>>+            s->decoded[channel][i] += sum >> qlevel;
>>>+        }
>>>+    } else {
>>>+        int sum;
>>>+        for (i = pred_order; i < s->blocksize; i++)
>>>+        {
>>>+            sum = 0;
>>>+            for (j = 0; j < pred_order; j++)
>>>+                sum += coeffs[j] * s->decoded[channel][i-j-1];
>>>+            s->decoded[channel][i] += sum >> qlevel;
>>>+        }
>>
>>sorry...I do realize that I am speaking up a bit late, but I have a
>>question about this change.  I don't see the reason to have 2 versions
>>of this.  I know that having a 32-bit sum for 16-bit audio is not likely
>>to overflow, but it's not a guarantee, and with lossless audio it's
>>critical to be 100% sure.  My suggestion would be to always use the
>>64-bit version.  This would put the overflow limit out of the range of
>>theoretical possibility.
> 
> 
> Overflow is not an error. For 16bit audio, if qlevel<=16, then bit #32 of 
> the sum will never be used, no matter whether it's stored in a 64bit 
> variable or just discarded.
> OK, so maybe the check should be if(s->bps + qlevel > 32)
> 
> --Loren Merritt

The thing that concerns me is that while the audio itself is guaranteed
to be signed 16 (or 17)-bit, the level-shifted predicted value (final
sum) is not.  A bad predictor can generate >32-bit values.  An encoder
would be unlikely to use parameters which cause this due to them not
giving good compression, but it is not constrained by the spec.  For
example, in the FLAC reference encoder there is a compile-time option to
check for the range of this value.  If turned on, it uses a 64-bit sum
and generates a warning if the value is >32-bit.  In my encoder, I
always use a 64-bit sum and return an error value (instead of issuing a
warning) to indicate not to encode with those parameters.  Neither of
these solutions is in the spec though.  An encoder would still be within
spec if it chose not to do any check, keep the predicted value & even
the final residual in 64-bit storage, and encode the result in the FLAC
file.

-Justin