flac dev - Apr 2015 - "keep qlp coeff precision such that only 32-bit math is required"

Ok, I just did a comparison of 1.2.1 with 1.3.2, and the change you're
suggesting was already there before. So, now the question becomes: why was the
code changed in the first place? Was there a bug that was fixed by changing 17
to 16, or did someone just get overzealous in a code review and thought that 17
was a bad choice? Perhaps 32 bits isn't actually large enough to handle the
overhead of processing 17 bit values - I haven't done the analysis to say
whether it's safe or not. I do recall discussions that might have been
referring to this sort of thing, so look at the change log to see why the change
came about.

Brian Willoughby
Sound Consulting


On Apr 18, 2015, at 12:20 PM, Brian Willoughby <brianw at
audiobanshee.com> wrote:
> Hmm, I don't know the history of the code, but flac 1.2.1
stream_encoder.c has
> 
> 									min_qlp_coeff_precision = FLAC__MIN_QLP_COEFF_PRECISION;
> 									/* try to ensure a 32-bit datapath throughout for 16bps(+1bps for
side channel) or less */
> 									if(subframe_bps <= 17) {
> 										max_qlp_coeff_precision = min(32 - subframe_bps - lpc_order,
FLAC__MAX_QLP_COEFF_PRECISION);
> 										max_qlp_coeff_precision = max(max_qlp_coeff_precision,
min_qlp_coeff_precision);
> 									}
> 
> in process_subframe_(), whereas evaluate_lpc_subframe_() has the comments
that you refer to.
> 
> I've not done an analysis to determine whether these constants should
match, or if maybe they should remain distinct.
> 
> Brian Willoughby
> 
> 
> On Apr 18, 2015, at 8:05 AM, lvqcl <lvqcl.mail at gmail.com> wrote:
>> stream_encoder.c has the following code:
>> 
>> 
>> 	/* try to keep qlp coeff precision such that only 32-bit math is
required for decode of <=16bps streams */
>> 	if(subframe_bps <= 16) {
>> ...
>> 
>> 
>> But FLAC can convert 16-bit input to 17-bit if mid-side coding is used.
>> So, does it make sense to compare subframe_bps with 17?

Brian Willoughby wrote:
> Ok, I just did a comparison of 1.2.1 with 1.3.2, and the change you're
> suggesting was already there before. So, now the question becomes: why
> was the code changed in the first place?
There should be some indication of why in the git history.

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

Erik de Castro Lopo wrote:
> There should be some indication of why in the git history.
http://git.xiph.org/?p=flac.git;a=commitdiff;h=27846708fe6271e5e3965a4bbad99baa1ca24c49

Now I remember a discussion about a bug in -p switch: the old code substracts
lpc_order instead of FLAC__bitmath_ilog2(lpc_order), and this commit fixes this.

It seems that the logic in process_subframe_() and in evaluate_lpc_subframe_()
is the same, so the constants in the conditions should be the same: either
both equal to 16, or both equal to 17.



I built two flac executables, the 1st is from current git head, the 2nd - with
my patch applied. Then I took a 16-bit WAV file and compressed it with both
encoders.

Standard compression levels (-0...-8): no difference (the resulting files are
bit-identical).
-p option: no difference (the resulting files are bit-identical).
-q N option (N = 13...15): the 2nd encoder produces slightly smaller files.
-l N --lax option (N = 16...31): again, the 2nd encoder produces slightly
smaller files.

About decoding speed: the difference isn't very big.
Except for "-l N --lax" option and 32-bit decoder: the files created
by the 2nd
encoder have noticeably faster decompression speed.

So it seems that my patch is slightly beneficial for those who use
non-standard encoding presets. Especially when they use non-Subset
encoding options.