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

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?

(The patch is attached. What do you think about it?)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bps.patch
Type: application/octet-stream
Size: 1299 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/flac-dev/attachments/20150418/9ead9436/attachment.obj

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?
>

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?