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 _______________________________________________ flac-dev mailing list flac-dev@xiph.org http://lists.xiph.org/mailman/listinfo/flac-dev
