Apply and test patch to convert internal buffers to floats

[njh]

Original tracker ticket: https://sourceforge.net/tracker/?func=detail&aid=1760298&group_id=136040&atid=735435

Patch here: https://gist.github.com/762513

I've attached a patch, that enables internal FLOAT buffer support. All internal 16 bit code has been replaced.

Should be compatible with existing interface.

It also replaces:

---

if ((filter[i][k] = 1e9 * cos ((FLOAT) ((2 * i + 1) * k * PI64))) >= 0)
  modf (filter[i][k] + 0.5, &filter[i][k]);
else
  modf (filter[i][k] - 0.5, &filter[i][k]);

---

With:

if ((filter[i][k] = 1e9 * cos ((FLOAT) ((2 * i + 1) * k * PI64))) >= 0) {
  filter[i][k] = (int)(filter[i][k] + 0.5);
} else {
  filter[i][k] = (int)(filter[i][k] - 0.5);
}

---

Since the former triggers a compiler error on MSVC++ 8, making it overwrite values in global_opt->smem->off[0].

Right now the internal buffer is the same as FLOAT is defined as in common.h - changing it from double to float results in a significant slowdown on windows - probably because there are a lot of float <-> double conversions needed. Functions would need to be replaced by float ones.

There are still some places that should be investigated:

psycho_1.c - Line 533 & psycho_1.c - Line 128: max = 20 * log10 (scale[i] * 32768) - 10; /* level for each subband */

ath.c, line 98 - guess that one is for general confusion.

Regards, Klaus Post.

[njh]

@eblanca what do you think of this?

[eblanca]

I will need to study various parts of the code and see what's going to change with this patch.

[eblanca]

I downloaded the patch and I'm reading it, but cannot reach the page https://sourceforge.net/tracker/?func=detail&aid=1760298&group_id=136040&atid=735435 can you ?

[njh]

Just checked and I think SourceForge deleted the old issue tracker. Hopefully I copy/pasted all the useful details across.

Thanks for looking at this.

[eblanca]

So, this code should replace short integer with double precision float. Apart from working/not working topics, what was the aim of the patch author? Will this increase encoding precision/quality ? Encoding speed ? Will this be useful looking forward to future twolame improvements ? Without a solid reason, I don't see the point in changing something that's currently working.

[njh]

I guess one of benefits is to reduce the number of times of converting between integer and floats and the errors that may introduce.

@klauspost were you the original submitter? (many years ago!)

[klauspost]

Indeed, must be 10 years ago. It was added, so input could be float numbers normalized to 1.0 and not have back/forth conversions.

[eblanca]

What about the current twolame API? The existing encoding routines are for short int samples and 32bit floating point samples, so I figure out some new "twolame_encode_buffer_double*" should be created, should it?

[njh]

Yes, a floating point API was on my todo list at one point.

Lame has:

int CDECL lame_encode_buffer_float(
        lame_global_flags*  gfp,           /* global context handle         */
        const float         pcm_l [],      /* PCM data for left channel     */
        const float         pcm_r [],      /* PCM data for right channel    */
        const int           nsamples,      /* number of samples per channel */
        unsigned char*      mp3buf,        /* pointer to encoded MP3 stream */
        const int           mp3buf_size ); /* number of valid octets in this

[eblanca]

This turned out to be a challenging (yet interesting!) job. I applied the patch in a branch of my twolame git repository, so you can have a look there. After an overall code review, my free thoughts with no particular order:

• using floating point math internally would mean the actual short int api (twolame_encode_buffer and twolame_encode buffer_interleaved) will be abandoned, won't it?
• the actual floating point interface (two encoding functions named as above and terminating with a "_float32") does the job on single precision samples, but if the inner part of the library does use double precision, then a double precision interface will be needed. At this point, will this single precision interface still make sense? With a single precision frontend and a double precision encoding, the conversion slow down will still be there.
• the libsndfile api offers both single precision samples and double precision samples reading facilities, with the added value of an internal conversion for any different sample format.
• in order to avoid the conversion slow down, two encoding library cores may be built: one for single precision encodings and one for double precision encodings.
• in order not to blow up the library size, a single library core may be built, using a precision level (single/double) chosen at build time. With this in mind, there will be no point in maintaining a double encoding interface (single AND double precision), so only one will be offered. And will this BREAK api compatibility? I don't know, but I feel this solution very attractive. (There may be small embedded systems out there, which can only support single precision)
• my thoughts do not take into account neither performance considerations nor quality considerations. We all know single precision floating point numbers are faster than double point precision but will they reach the same quality? Will a flexible encoding library make sense? I don't know.

[njh]

Thank you for looking into this Elio.

1. No, the 'short int api' wouldn't be abandoned. The core code will become based on floats and the first thing that twolame_encode_buffer() will do is convert to floats (ie the opposite of what twolame_encode_buffer_float32() does now)

2. I am not sure about single precision v. double precision but I think it probably makes sense just to pick one. Probably single precision is fine for our purposes. jackd (which I have used quite a lot) uses 32-bit floating point values for all samples.

3. Yes, I think it would make sense for the frontend CLI to then switch to sf_read_float()

4. I am not sure of the value of having support for double precision.

5. Yes, I think having support for both, or having a compile-time option adds too much complexity.

One disadvantage to doing this is is that there may be some embedded systems that don't have native floating point support.

[eblanca]

In my branch move_to_float I am now able to select floating point precision once for all into common.h and then I get a library using only either single precision or double precision floating point numbers, called FLOAT (upper case) everywhere. This selection requires acting on a single macro! In order to take advantage from this flexible new library, I will work a bit on the frontend replacing the "old" signed integer interface with this above mentioned FLOAT interface. And then quality AND speed issues will need to be evaluated.

[eblanca]

My branch 'move_to_float' https://github.com/eblanca/twolame/tree/move_to_float is now in a working clean state, would you please give it a look and validate the new encoding engine?

Edit: the FLOAT type is now 'float' (32 bit)