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

2014-07-04 Thread Erik de Castro Lopo
Miroslav Lichvar wrote: In the precompute_partition_info_sums_ function, instead of selecting 64-bit accumulator when the signal bps is larger than 16, revert to the original approach based on partition size, but make room for few extra bits to not overflow with unusual signals where the

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

2014-06-30 Thread Erik de Castro Lopo
lvqcl wrote: As I see it: FLAC 1.2.1 and 1.3.0 cannot encode snippet6.wav with -7 and -8 encoding modes. But they are able to do this with --disable-fixed-subframes option. This implies that snippet6.wav triggers a problem somewthere inside FLAC__fixed_compute_residual(data[], data_len,

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

2014-06-30 Thread Miroslav Lichvar
On Mon, Jun 30, 2014 at 08:08:42PM +1000, Erik de Castro Lopo wrote: Can someone please provide me with a copy of the file snippet6.wav ? I'd like to test this some more. I got it from http://wootangent.net/~lsd/blah/snippet6.wav -- Miroslav Lichvar

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

2014-06-30 Thread lvqcl
Miroslav Lichvar wrote: Adding the extra bits to bps in evaluate_fixed_subframe_ instead of precompute_partition_info_sums_ is ok with me, that's what I suggested in the original thread discussing this problem, found it: http://lists.xiph.org/pipermail/flac-dev/2013-July/004303.html but I

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

2014-06-27 Thread Erik de Castro Lopo
Miroslav Lichvar wrote: In the precompute_partition_info_sums_ function, instead of selecting 64-bit accumulator when the signal bps is larger than 16, revert to the original approach based on partition size, but make room for few extra bits to not overflow with unusual signals where the

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

2014-06-20 Thread Miroslav Lichvar
On Thu, Jun 19, 2014 at 09:23:26PM +0200, Martijn van Beurden wrote: op 19-06-14 20:17, lvqcl schreef: It seems quite common for 16-bit files: I second that. Apparently it depends on the kind of music, some albums have no warnings (mostly classical music it seems), some have over 20 per

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

2014-06-20 Thread Miroslav Lichvar
On Fri, Jun 20, 2014 at 01:21:03PM +0400, lvqcl wrote: Miroslav Lichvar цкщеу: +/* + * This is used to avoid overflow with unusual signals in 32-bit + * accumulator in the *precompute_partition_info_sums_* functions. + */ +#define FLAC__MAX_EXTRA_RESIDUAL_BPS 4 + /*

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

2014-06-20 Thread Miroslav Lichvar
On Fri, Jun 20, 2014 at 02:15:31PM +0400, lvqcl wrote: Miroslav Lichvar wrote: As overflow in the accumulator won't result in a data loss, I think this is good enough until someone can figure out a better approach. FLAC calculates real bitdepth of input signal for every block. Is it

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

2014-06-19 Thread lvqcl
Miroslav Lichvar wrote: In the precompute_partition_info_sums_ function, instead of selecting 64-bit accumulator when the signal bps is larger than 16, revert to the original approach based on partition size, but make room for few extra bits to not overflow with unusual signals where the

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

2014-06-19 Thread 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) *

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

2014-06-19 Thread lvqcl
Miroslav Lichvar wrote: 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

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

2014-06-19 Thread Miroslav Lichvar
On Thu, Jun 19, 2014 at 03:30:06PM +0200, Miroslav Lichvar wrote: 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? I think it would be interesting to know

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

2014-06-19 Thread Miroslav Lichvar
On Thu, Jun 19, 2014 at 05:04:23PM +0200, Miroslav Lichvar wrote: Yes, it's the same check. Assuming residual can be at most FLAC__MAX_EXTRA_RESIDUAL_BPS bits wider than subframe_bps, I think it should be: if(subframe_bps + qlp_coeff_precision + FLAC__bitmath_ilog2(order) +

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

2014-06-19 Thread lvqcl
Miroslav Lichvar wrote: I think it would be interesting to know how common are such streams. I patched flac to print a warning on decoding or testing when this is detected, but didn't find any files with this problem in my (small) music collection. If someone has a large collection and some

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

2014-06-19 Thread Martijn van Beurden
op 19-06-14 20:17, lvqcl schreef: It seems quite common for 16-bit files: I second that. Apparently it depends on the kind of music, some albums have no warnings (mostly classical music it seems), some have over 20 per file. I've seen a few with 100 per file (things like rock and heavy