Change Soundsource API to reading float samples

[Daniel Schürmann]

All samples read from a specific sound source are immediately converted from S16 Samples to float samples.
Some track encoding formats original have float samples. In this case we have two unnecessary sample conversations.

[RJ Skerry-Ryan (rryan)]

Definitely agree and it would allow higher precision soundsources.

On Mon, Mar 18, 2013 at 7:31 AM, Daniel Schürmann <
<email address hidden>> wrote:

> Public bug reported:
>
> All samples read from a specific sound source are immediately converted
> from S16 Samples to float samples.
> Some track encoding formats original have float samples. In this case we
> have two unnecessary sample conversations.
>
> ** Affects: mixxx
> Importance: Wishlist
> Status: New
>
> ** Changed in: mixxx
> Importance: Undecided => Wishlist
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1156569
>
> Title:
> Change Soundsource API to reading float samples
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1156569/+subscriptions
>

[James Fagan (jfagan1)]

I have tried to take on this bug as my first contribution to mixxx and so far I have compiled mixxx and read over the various soundsource.cpp files and came across the line of code......float bpm = sBpm.toFloat(); in soundsource.cpp. In the method str2bpm I have come up with a few ideas on how to check if the Qstring is actually a float. Then I noticed in the title of this bug it stated to Change Soundsource API to reading float samples. I am not really sure if the API means these .cpp files or is there something else I need to be looking at. If there is some API that I need to edit can someone please tell me where to find it in the folders? Thanks.

[RJ Skerry-Ryan (rryan)]

Hi James,

In this case we're referring to the SoundSource base class as the API and
it is defined in src/soundsource.h.

You'll see that the read() method reads SAMPLE's which is just a typedef
for 16-bit integer. What we'd like is that this method deals with floats
(CSAMPLE is our standard typedef for floating point samples) instead of
SAMPLE. This way, sound sources that have higher resolution versions of the
samples don't have to down-res to 16-bit only for the data to be
re-converted back to float in the engine.

The rest of Mixxx (e.g. CachingReader -- src/cachingreader.cpp) uses
SoundSourceProxy to initialize a particular SoundSource. You can just
search for all references to SoundSourcePRoxy to see every usage of
SoundSource by other parts of Mixxx.

Hope that helps,
RJ

On Wed, Apr 17, 2013 at 12:22 AM, James Fagan <email address hidden>wrote:

> I have tried to take on this bug as my first contribution to mixxx and
> so far I have compiled mixxx and read over the various soundsource.cpp
> files and came across the line of code......float bpm = sBpm.toFloat();
> in soundsource.cpp. In the method str2bpm I have come up with a few
> ideas on how to check if the Qstring is actually a float. Then I
> noticed in the title of this bug it stated to Change Soundsource API to
> reading float samples. I am not really sure if the API means these .cpp
> files or is there something else I need to be looking at. If there is
> some API that I need to edit can someone please tell me where to find it
> in the folders? Thanks.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/1156569
>
> Title:
> Change Soundsource API to reading float samples
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1156569/+subscriptions
>

[James Fagan (jfagan1)]

Thank you RJ Ryan your post was very helpful. I have just finished going through all of the files that use soundSourceProxy and have changed them accordingly. I then compiled the new changes successfully and I believe I have properly changed everything but I am a little unsure how to test to check that original float samples are not being converted.

[Owen Williams (ywwg)]

As a start, can you create a diff of your work so I can see what you've changed? If you're using a bzr checkout the "bzr diff" command should generate a file we can use. Just attach it to this bug and I'll take a look.

[James Fagan (jfagan1)]

I have attached a txt of the diff result text file. Let me know if you need anything else. Thanks

[Daniel Schürmann (daschuer)]

Hi James,

Thank you for the patch.

I am afraid this will neither work, nor compile.

Are your fermilar with setting a up a C++ project? What is your operating
system, and your favorite IDE?

First of all you should set up a powerful build environment.

The basic steps are described here:
http://mixxx.org/wiki/doku.php/start#build_mixxx

If you have the choice, setup your build environment using Linux for
"minimum pain".

Than you should try to setup gdb, that you are able to single step thought
the code.
You will find in my trunk the .gdb file I am using and also Eclipse project
files.

If you get stuck, please ask.

Kind regards,

Daniel

2013/4/18 James Fagan <email address hidden>

> I have attached a txt of the diff result text file. Let me know if you
> need anything else. Thanks
>
> ** Attachment added: "differences.txt"
>
> https://bugs.launchpad.net/mixxx/+bug/1156569/+attachment/3647028/+files/differences.txt
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1156569
>
> Title:
> Change Soundsource API to reading float samples
>
> Status in Mixxx:
> New
>
> Bug description:
> All samples read from a specific sound source are immediately converted
> from S16 Samples to float samples.
> Some track encoding formats original have float samples. In this case we
> have two unnecessary sample conversations.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1156569/+subscriptions
>

[James Fagan (jfagan1)]

Hey Daniel,

Thank you for all the useful tips. I am currently using Ubuntu and I normally do not use an IDE, just kate and geedit. I have used eclipse before with java so maybe I can give that a try. I have followed the steps in the link you provided and set up mixxx with linux with no issues. When I tried to compile the code it was unsuccessful due to errors. I could of sworn it compiled fine yesterday but I apologize for posting something that does not compile. I will be sure to continue working on this ASAP. Also, I am familiar with gdb but I am unfamiliar how to retrieve your files from your trunk.

Thanks,
James

[Daniel Schürmann (daschuer)]

Hi James,

Ah fine, that you are setup so far.

I have just put some additional hints about eclipse to our Wiki
http://www.mixxx.org/wiki/doku.php/eclipse

The other thing is getting familiar with bazaar
http://doc.bazaar.canonical.com/latest/en/mini-tutorial/

If you want to provide a patch next time it is best using bzr from the
command line like:
bzr diff > name.patch

The solution of this Bug should include to move the call to
SampleUtil::convert() into the SoundSources if required.
http://bazaar.launchpad.net/~mixxxdevelopers/mixxx/trunk/view/head:/mixxx/src/sampleutil.cpp#L643

And .. please assign the bug to yourself

2013/4/18 James Fagan <email address hidden>

> Hey Daniel,
>
> Thank you for all the useful tips. I am currently using Ubuntu and I
> normally do not use an IDE, just kate and geedit. I have used eclipse
> before with java so maybe I can give that a try. I have followed the
> steps in the link you provided and set up mixxx with linux with no
> issues. When I tried to compile the code it was unsuccessful due to
> errors. I could of sworn it compiled fine yesterday but I apologize for
> posting something that does not compile. I will be sure to continue
> working on this ASAP. Also, I am familiar with gdb but I am unfamiliar
> how to retrieve your files from your trunk.
>
> Thanks,
> James
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1156569
>
> Title:
> Change Soundsource API to reading float samples
>
> Status in Mixxx:
> New
>
> Bug description:
> All samples read from a specific sound source are immediately converted
> from S16 Samples to float samples.
> Some track encoding formats original have float samples. In this case we
> have two unnecessary sample conversations.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/1156569/+subscriptions
>

[James Fagan (jfagan1)]

I apologize for all the questions but I am still somewhat confused about how I actually determine whether a sample is of type SAMPLE or CSAMPLE. Also to clarify the problem I am trying to fix is that everytime a sample is brought into mixxx it is automatically read in by soundsource.h 's read method as a SAMPLE and then everywhere else in the code that sample may be used it must be converted to CSAMPLE. This is the problem because if we have a file that is orignally above 16 bits (CSAMPLE) it is converted to SAMPLE by the read method and then back to CSAMPLE later; so we have 2 extra conversions for no reason. What I don't understand is when a sample is brought into mixxx, how can I just read it in as a CSAMPLE or a SAMPLE if I don't actually know what it is?

Thanks,
James

[Daniel Schürmann (daschuer)]

Hi James,

often the underlying sound source Libs have a internal converter.
Currently all SoundSouces are providing SAMPLES, the solution is different in each SoundSource.

So a first step is to call SampleUtil::convert() from the sound Source itself and remove it from everywhere else in Mixxx.

The second step is to find out how to receive CSAMPLES from the underlying libs to get rid of the SampleUtil::convert() calls within the libraries.

http://portaudio.com/docs/v19-doxydocs/portaudio_8h.html#a4582d93c2c2e60e12be3d74c5fe00b96

[RJ Skerry-Ryan (rryan)]

Hi James,

Every sound API's results get passed through convert() by CachingReader. This does nothing but re-interpret the signed 16-bit samples as floating point. The values of the samples themselves do not change in this conversion.

There's no need to detect whether a file is natively encoded in floating point samples or signed 16-bit samples. Currently, every SoundSource implementation produces s16 samples no matter what the native file format is. You just need to change our use of each API to produce float samples instead of s16 samples.

For SoundSourceMp3, as Daniel mentioned, madScale is that conversion step. It takes a mad_fixed_t sample and produces a signed integer.

One easy way to accomplish what you're looking to do is to just make something like madScale return a float. This would implicitly convert the calculated s16 value to float upon return because of C++ type coercion. However, this is wasteful. Ideally you will find a way to convert a mad_fixed_t directly to a float rather than mad_fixed_t -> s16 -> float. Daniel suggested looking at the libmad documentation / code to figure out how to do this.

Hope this helps,
RJ

[James Fagan (jfagan1)]

Hey all,

I have been doing a lot of research on fixed and floating point numbers and have learned the following:

A floating point number has 1 bit for sign followed by 8 bits for exponent and a 23 bit mantissa
A mad_fixed_t has 1 bit for sign followed by 3 bits for the whole number portion and 28 bits for the fractional portion.

To convert a 32 bit mad_fixed_t to a 32 bit float I have created a new madScale function in soundsourcemp3.cpp that I included in mp3.patch. The good news is that when I run mixxx and drag an mp3 onto the deck I do hear the song playing but the problem is that I also have a lot of extra noise in the background as well. I have determined that the problem is how I am assigning an exponent value to the float. I orignally thought the value for the exponent binary would be a constant but I have determined that is very wrong. What I am a having a lot of trouble determining is how I can manipulate the mad_fixed_t to determine the 8 bit exponent necessary for floating point. The chart below represents the realtionship I am trying to find a function for:

Mad_fixed_t value 8 bit Exponent binary for Float
  0.0 0
  1.something 127
  2.something 128
  3.something 128
  4.something 129
  5.something 129
  6.something 129
  7.something 129
 .10 to .128888 123
.129 to .249999 124
etc

I have attached a patch file with my new additions and in my madScale function I have included numerous comments explaining my thought process for each line. I believe that once I find out how to properly assign the 8 bit exponent then madScale should work properly. If you have any questions about my function or thought process please feel free to ask.

Thanks,
James Fagan

[James Fagan (jfagan1)]

I have attached the .cpp of the detailed example to this comment.

[James Fagan (jfagan1)]

I apologize for making so many posts but I have accidently hid comment #14 and since I left the page there was no way for me to unhide it. Also I have made more progress since posts #13 and #14 and I would like to edit or even delete them so that no one wastes time trying to help me fix an outdated problem. If there is a way to edit a post could someone please tell me how?, that way I could update my progess in the same comment so it doesnt make a mess on launchpad. As of right now I have got passed the error mentioned in post 14 and issues in post 13 and when I run mixxx and drag an mp3 onto the deck the song does play but there is still some noise but definitely much less then before so I must be close. I have attached the my newest patch file with my additons to madScale. Also the attached file in post #15 is still an up to date example of going through my code. Once again I appologize for posting so many times back to back but I realized how to fix the problem in post #14 a little bit after I posted it and I didnt want anyone to waste time trying to help me.

Thanks,
James Fagan

[Daniel Schürmann (daschuer)]

Hi James,

It looks like you learned a lot about bit interpretation of different data types.
While I have not completely tracked down your changes, it looks like you get caught by the issues of the original code.

// Original fixed and commented
inline SAMPLE SoundSourceMp3::madScale(mad_fixed_t sample) // actually returns SAMPLE = int16_t, return in int was wrong.
{
    sample += (1L << (MAD_F_FRACBITS - 16)); // += 0x1000 (+0,5 to avoid rounding errors)

  // overflow protection
  if (sample >= MAD_F_ONE) {
         // camp to sample < 1
         sample = MAD_F_ONE - 1;
   }
   else if (sample < -MAD_F_ONE) {
       // clamp to sample >= -1
       sample = -MAD_F_ONE;
  }
    return sample >> (MAD_F_FRACBITS + 1 - 16); // /= 0x2000
}

// Straight forward solution, only move convert
// drawback: loss of precision and clamping
inline CSAMPLE SoundSourceMp3::madScale(mad_fixed_t sample)
{
    sample += (1L << (MAD_F_FRACBITS - 16));

   if (sample >= MAD_F_ONE)
       sample = MAD_F_ONE - 1;
   else if (sample < -MAD_F_ONE)
       sample = -MAD_F_ONE;

   return static_cast<CSAMPLE>(sample >> (MAD_F_FRACBITS + 1 - 16));
}

// Better Solution, (untested):
inline signed float SoundSourceMp3::madScale(mad_fixed_t sample)
{
     double dsample = mad_f_todouble(sample);
     dsample *= 0x8000;
     return static_cast<CSAMPLE>(dsample);
}

// Faster Solution on 32 bit ach, (untested):
inline signed float SoundSourceMp3::madScale(mad_fixed_t sample)
{
     return = static_cast<CSAMPLE>(sample) / static_cast<CSAMPLE>(1L << (MAD_F_FRACBITS - 15))
}

[Daniel Schürmann (daschuer)]

By the way, I wonder why the Mixxx CSample is multiplied by 0x8000;

http://freedesktop.org/software/pulseaudio/doxygen/sample.html

maybe there is somewhere a downscaling to 1 .. -1 we could eliminate.

[James Fagan (jfagan1)]

Hey Daniel,

Thanks so much for clearing that up, I tested your (Better Solution) and every mp3 I tried played correctly! I can't believe all I had to do was use the mad_f_todouble method. I orignally had that in my earlier attempts but I ruled it out because I wasn't hearing any sound but that *= 0x0800 was the trick. I honestly don't undertand why that was necessary but I can look into the downscaling thing you mentioned to see why. As for all the other soundsources I am having trouble finding what I need to change. Just to test out where I am at I tried importing and playing various file types and seeing if they play correctly. I was able to play .flac sucessfully but .ogg, aiff, and .m4a all produced some sort of static. Also if you could calrify what types of files I need to test for each soundSource that would aslo be a huge help. Earlier you also mentioned that the solution of this Bug should include to move the call to SampleUtil::convert() into the SoundSources if required. For soundSource mp3 all I changed was the madScale method and changed SAMPLE to CSAMPLE but do I still need to call convert somewhere? or is madScale what you meant by convert?

Thanks again for all your help,

James

[Daniel Schürmann (daschuer)]

Hi James,

Did you also test "Faster solution". Something like this should finally move to trunk.
SoundSourceMp3::madScale is now a replacements for convert().

I am not sure if you finaly got the point with moving convert. So here a pseudo code example, which reads a German String "msgDe" from an external Library "getMsgLib()" and prints a translated English version to terminal.

// Before;
StrDe readMsg() {
    StrDe msgDe = getMsgLib();
    return msgDe;
}
int main() {
    StrDe msgDe = readMsg();
    StrEn msgEn = convert(msgDe);
    cout << msgEn;
    return 0;
}

// Your patch version
StrEn readMsg() {
    StrDe msgDe = getMsgLib();
    return msgDe; // Implicit faulty type conversion
}
int main() {
    StrDe msgEn = readMsg();
    cout << msgEn;
    return 0;
}

// final version
StrEn readMsg() {
    StrDe msgDe = getMsgLib();
    StrEn msgEn = convert(msgDe); // convert has moved
    return msgEn;
}
int main() {
    StrDe msgEn = readMsg();
    cout << msgEn;
    return 0;
}

// Now a more complex version similar to SoundsourceMp3. Lets say the external library speaks French.
// Main is still expecting a German String in the "Before" version, so the same as in the example above.

// Before;
StrDe readMsg() {
    StrFr msgFr = getMsgFrLib();
 StrFr msgFr = getMsgFrLib();

    return msgDe;
}
int main() {
    StrDe msgDe = readMsg();
    StrEn msgEn = convert(msgDe);
    cout << msgEn;
    return 0;
}

[Daniel Schürmann (daschuer)]

Sorry I have accidentally hit Enter, so the Second example was unfinished.
Her it is :

// Now a more complex version similar to SoundsourceMp3. Lets say the external library speaks French.
// Main is still expecting a German String in the "Before" version, so the same as in the example above.

// Before;
StrDe readMsg() {
    StrFr msgFr = getMsgFrLib();
    StrDe msgDe = toDe(msgFr);
    return msgDe;
}
int main() {
    StrDe msgDe = readMsg();
    StrEn msgEn = convert(msgDe);
    cout << msgEn;
    return 0;
}

// After;
StrEn readMsg() {
    StrFr msgFr = getMsgFrLib();
    StrEn msgEn = toEn(msgFr); // new optimized conversion function
    return msgEn;
}
int main() {
    StrDe msgEn = readMsg();
    cout << msgEn;
    return 0;
}

You see, convert is gone and there is no also msgDe reqired.

So you see, moving convert will produce allays a working version without knowledge of the Languages (Sound source interns)
In the second example you need to write a new function toEn() (madScale())

[James Fagan (jfagan1)]

Hey Daniel,

Thank you for those pseudo code examples , they helped me understand convert much better. Also I have tried your Faster Solution for madscale and it worked successfully! I was on 64 bit Ubuntu with 64 bit MIxxx and it worked fine even though you mentioned it was for 32 bit. I have given it a good shot in trying to implement the same idea in your pseudo code in order to work on the sounsourceflac.cpp but I am still running into some trouble. I have attached the patch that has the working soundsourcemp3 and the changes I have made to soundsourceflac. One thing I was noticing though was no matter what I changed as long as I kept the method getShift() in soundsourceflac.cpp return 16-m_bps then flac files would play properly in Mixxx. I thought since that 16 refers to the 16 bits for Mixxx's SAMPLE type we would need it to be 32- m_bps to deal with CSAMPLE. My other idea was to get rid of the whole getShift() method completely because a FLAC supports from 4 to 32 bits per sample and since we want to return a 32 bit float from SoundSourceFLAC::shift(FLAC__int32 sample) we would not need any shifting; we would only need to cast sample as a CSAMPLE and return it.

Thanks,
James

[Daniel Schürmann (daschuer)]

Hi James,

it would be nice if you could provide a straight forward patch first, which includes the convert move solution like
comment #20 "final version".
You should also get rid of the +x changes and please remove old code in comments.
One approach for this is a second fresh checkout and copy your changes using "meld" on Linux hosts.

This way we have a working solution and can finish tis long thread. The other optimizations can be done with a new bug.

--

float = 32 bit; double = 64 bit. A 64 bit controller can handle a double at once in all its registers. A 32 bit controller needs two cycles for some 64 bit operations. Of cause a 64 bit controller can handle 32 bit values fast!
So in respect to 32 bit builds, it is a good idea to avoid double if the double precision is not required.

--

> we would only need to cast sample as a CSAMPLE and return it.
Yes, this would be a solutions if we normalize our CSAMPES to -1 .. 1. This seems the common method outside Mixxx ;-)
But until we have not fixed it we need a "*= 0x8000;"

For int "x <<= 1" is the same as "x *= 2"
But this is not true for floats and double.

--

[James Fagan (jfagan1)]

Hey Daniel,

I have attached the straight foward patch you requested. I tried what you said in your last comment about "*= 0x8000;" for soundSourceFlac.cpp but that aslo didnt work. I just recently got a full time job for the summer and unfortunately I will not be able to work much more on this bug at this time. I really appreciate everyones help to get me this far;I have learned so much in this past month working with Mixxx and as soon as I get the chance I will continue working on Mixxx related projects and bug fixing.

Thanks,
James Fagan

[Daniel Schürmann (daschuer)]

Here is Bug #1204039 for the "*= 0x8000;" thing.

[Uwe Klotz (uklotzde-deactivatedaccount)]

https://github.com/mixxxdj/mixxx/pull/411

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6947