[flac-dev] [PATCH] stream_encoder : Improve selection of residual accumulator width

[Miroslav Lichvar]

On Thu, Jun 19, 2014 at 03:30:22PM +0400, lvqcl wrote:
> BTW, what can you say about the following place in stream_decoder.c
> in read_subframe_lpc_() function:
> 
>      /*@@@@@@ technically not pessimistic enough, should be more like
>      if( (FLAC__uint64)order * ((((FLAC__uint64)1)<<bps)-1) * ((1<<subframe->qlp_coeff_precision)-1) < (((FLAC__uint64)-1) << 32) )
>      */
>      if(bps + subframe->qlp_coeff_precision + FLAC__bitmath_ilog2(order) <= 32)
> 
> Is it really "not pessimistic enough"? Can it be changed similarly to your patch
> for stream_encoder.c?

Good question. I'm not sure what exactly Josh meant by that comment.
The git message says just "minor comments".

I think the right size of the expression was meant to be
(FLAC__uint64)1<<32, otherwise it doesn't make much sense to me. With
that it can rewritten in log2 as

bps + subframe->qlp_coeff_precision + FLAC__bitmath_ilog2(order - 1) < 32

This is indeed more pessimistic that the currently used check (< 32 vs
<= 32), but I think both make a mistake that the qlp coefficients and
residuals are unsigned integers and are actually more pessimistic than
they would need to be if residuals were at most bps wide. With signed
multiplication I think the correct check would actually be

bps + subframe->qlp_coeff_precision + FLAC__bitmath_ilog2(order) - 1 <= 32

But, as we have seen with unusual data the residual signal can be
wider than bps. The FLAC format specification doesn't seem to mention
this. Should it be treated as a valid FLAC stream? Based on the
analysis above, the currently used check allows residuals at most 1
bit wider than bps. Another problem could be that the
local_lpc_restore_signal_16bit function may truncate the residual to
16 bits.

-- 
Miroslav Lichvar