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

Erik de Castro Lopo wrote:
>> It slightly improves the performance with standard encoding levels and
>> 16-bit files as the 17-bit side channel can still be processed with the
>> 32-bit accumulator and correctly selects the 64-bit accumulator with
>> very large 16-bit partitions.
>>
>> This is related to commits 6f7ec60c and 187e596e.
>
> Sorry I wasn't able to deal with this patch when it came in because I
> was busy with $day_job.
>
> There was a lot of discussion about this patch but I can't really
> figure out from the thread what the conclusion was.
>
> The patch still applies, and the test suite passes. If there is a
> problem with this patch that the test suite doesn't catch, we should
> write a test that does catch it.
>
> If there is no problem with the patch I'll push it.
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, order, residual[]) function.

And indeed, max. possible residual value is 16 times bigger than max. value
in data[] array:

     residual[i] = data[i] - 4*data[i-1] + 6*data[i-2] - 4*data[i-3] + data[i-4]

16 == 2^4, so max. bps value for residual[] is equal to max. bps for data[] + 4.
The value of FLAC__MAX_EXTRA_RESIDUAL_BPS == 4 is enough to fix this problem
with FLAC__fixed_compute_residual().

lvqcl wrote:
> Erik de Castro Lopo wrote:
>
>>> It slightly improves the performance with standard encoding levels
and
>>> 16-bit files as the 17-bit side channel can still be processed with
the
>>> 32-bit accumulator and correctly selects the 64-bit accumulator
with
>>> very large 16-bit partitions.
>>>
>>> This is related to commits 6f7ec60c and 187e596e.
>>
>> Sorry I wasn't able to deal with this patch when it came in because
I
>> was busy with $day_job.
>>
>> There was a lot of discussion about this patch but I can't really
>> figure out from the thread what the conclusion was.
>>
>> The patch still applies, and the test suite passes. If there is a
>> problem with this patch that the test suite doesn't catch, we
should
>> write a test that does catch it.
>>
>> If there is no problem with the patch I'll push it.
>
> 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, order, residual[]) function.
>
> And indeed, max. possible residual value is 16 times bigger than max. value
> in data[] array:
>
>      residual[i] = data[i] - 4*data[i-1] + 6*data[i-2] - 4*data[i-3] +
data[i-4]
>
> 16 == 2^4, so max. bps value for residual[] is equal to max. bps for data[]
+ 4.
> The value of FLAC__MAX_EXTRA_RESIDUAL_BPS == 4 is enough to fix this
problem
> with FLAC__fixed_compute_residual().

On the other hand, it is possible to set FLAC__MAX_EXTRA_RESIDUAL_BPS
to 0, and change evaluate_fixed_subframe_() function instead: in the call
of find_best_partition_order_() function, replace subframe_bps
with subframe_bps + order.


...And maybe it's also better to use 'subframe_bps + order' instead
of 'order'
in calculation of 'estimate' variable in evaluate_fixed_subframe_()?

On Mon, Jun 30, 2014 at 03:27:18AM +0400, lvqcl wrote:
> lvqcl wrote:
> > 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, order, residual[])
function.
Interestingly, -8 -l 0 seems to avoid the encoding problem too.
> > And indeed, max. possible residual value is 16 times bigger than max.
value
> > in data[] array:
> >
> >      residual[i] = data[i] - 4*data[i-1] + 6*data[i-2] - 4*data[i-3] +
data[i-4]
> >
> > 16 == 2^4, so max. bps value for residual[] is equal to max. bps for
data[] + 4.
> > The value of FLAC__MAX_EXTRA_RESIDUAL_BPS == 4 is enough to fix this
problem
> > with FLAC__fixed_compute_residual().
> 
> 
> On the other hand, it is possible to set FLAC__MAX_EXTRA_RESIDUAL_BPS
> to 0, and change evaluate_fixed_subframe_() function instead: in the call
> of find_best_partition_order_() function, replace subframe_bps
> with subframe_bps + order.
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, but I think it should
be done also in the lpc function. Adding order instead of 4 might work
for fixed frames, for LPC it looks too pessimistic.
> ...And maybe it's also better to use 'subframe_bps + order'
instead of 'order'
> in calculation of 'estimate' variable in
evaluate_fixed_subframe_()?
Why exactly?

To me it looks like (order * subframe_bps) in the calculation is the
number of bits for the warmup samples in the fixed subframe.

-- 
Miroslav Lichvar

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, order, residual[]) function.
Can someone please provide me with a copy of the file snippet6.wav ? I'd
like to test this some more.

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

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