flac dev - Apr 2015 - About a comment in stream_decoder.c

I don't understand the comment in src/libFLAC/stream_decoder.c:

		/*@@@@@@ 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)


see
http://git.xiph.org/?p=flac.git;a=commitdiff;h=8534bbe4e92300609fd6dc289d882130b69d48cd



First, I suspect that there's a typo and the intended code is

		... < (((FLAC__uint64)1) << 32) )

(without an unary minus).


Second, this condition (in algebraic form)

	order * (2^bps - 1) * (2^qlp_coeff_precision - 1) < 2^32

makes sense if qlp_coeff[] and data[] in FLAC__lpc_restore_signal() are
unsigned integers. But they are signed, and the condition should be

	order * 2^(bps-1) * 2^(qlp_coeff_precision-1) < 2^31

which is equivalent to the current

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

So IMHO the comment is incorrect.

On Mon, Apr 20, 2015 at 08:00:20PM +0300, lvqcl wrote:
> I don't understand the comment in src/libFLAC/stream_decoder.c:
> 
> 		/*@@@@@@ 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)
> which is equivalent to the current
> 
> 	bps + subframe->qlp_coeff_precision + FLAC__bitmath_ilog2(order) <=
32
> 
> So IMHO the comment is incorrect.
Yeah, the current code looks right to me. I think we already discussed
this some time ago. I'd just remove that comment.

-- 
Miroslav Lichvar

Miroslav Lichvar wrote:
> I think we already discussed
> this some time ago.
I didn't fully understand the logic of this expressions at that time.
> I'd just remove that comment.
The patch is attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decoder_comment.patch
Type: application/octet-stream
Size: 1003 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20150424/5935b81b/attachment.obj