Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

outsource DSP functions #253

Open
andik opened this issue Jan 28, 2015 · 7 comments
Open

outsource DSP functions #253

andik opened this issue Jan 28, 2015 · 7 comments

Comments

@andik
Copy link

andik commented Jan 28, 2015

Hey guys,

as I look into the various (control-)generators of Tonic, I see this #ifdef __APPLE__ ... #else ... #endif stuff all around, for example in the ADSR. I personally think this makes the code hard to comprehend and hard to maintain/adapt.
I propose therefore a TonicDSP.h Headerfile which is used to abstract the DSP stuff away as much as possible...

for example (from ADSR)

namespace Tonic { 
  namespace dsp {
    inline void ramp(float start, float increment, float* buffer, int numSamples) {
      #ifdef USE_APPLE_ACCELERATE
        // starting point
        start += increment;
        // vector calculation
        vDSP_vramp(&start, &increment, buffer, 1, numSamples);
        // end point
        start += increment*(numSamples-1);
      #else
        for (int i=0; i<numSamples; i++){
          start += increment;
          *buffer++ = start;
        }
      #endif
    }
  }
}

another option would be macros:

#ifdef USE_APPLE_ACCELERATE
  #define TONIC_DSP_RAMP(start, increment, buffer, numSamples) \
    vDSP_vramp(&start, &increment, buffer, 1, numSamples);
#else
  #define TONIC_DSP_RAMP(start, increment, buffer, numSamples) \
    for (int i=0; i<numSamples; i++){                          \
      start += increment;                                      \
      *buffer++ = start;                                       \
    }
#endif

I prefer the first.

one could create that header, mark it as DSP Port Header and add the DSP Functions whenever one finds one. This way nearly no immediate efford is required. And it would also help to port Tonic to new platforms...

@andik
Copy link
Author

andik commented Jan 28, 2015

If you like, I'd create the file and start to port the stuff. Is mostly a matter of grepping for vDSP and outsource this all. BUT I do not know the Apple DSP functions, thus you'd have to look over my source before merging (which you will do anyway). Or you give me some tipps (using const float references as parameters maybe?)

@morganpackard
Copy link
Member

This sounds like a good idea to me. @ndonald2, what do you think? I'd rather not introduce another layer of function lookup overhead (even though I know the effect will be fairly negligible). I know macros won't introduce any function overhead. I suspect that any functions defined in a dsp header would be inlined, but I don't know for sure.

@ndonald2
Copy link
Member

Yes, this is a great idea. I had started down this road awhile back but never saw it through. Another good option might be to have multiple .cpp implementation files for the core DSP header that each use platform macros to compile the whole thing or nothing.

i.e.

// TonicDSP_apple.cpp
#ifdef USE_APPLE_ACCELERATE
#include "TonicDSP.h"
...
// All the implementations for the DSP functions
...
#endif

and so on.

@andik
Copy link
Author

andik commented Jan 28, 2015

@morganpackard
I'm not a native english speaker so I'm not shure if I understood you correctly. Did you mean that the compiler is free to not inline functions marked with inline? I heard that this is possible. But in my working place we inline a lot to have high performance and at least the DSP forwarding functions are so small they definitively get inlined. I'm not shure for the naive (for-loop) implementations. But there are so much calls, it doesn't matter ^^
I'd especcially go for inline functions because they are easier to use by the end-user and they raise better errors. Using macros was my first Idea, but I'd prefer not to use them.

@ndonald2
this is a good idea, except that we'd loose inlining, which may be not problem for the naive (for-loop) implementation of the functions.

So maybe we could use something like

// TonicDSP.h
#ifdef USE_APPLE_ACCELERATE
#include "TonicDSP_Apple.h"
...
// other Systems
...
#else
#include "TonicDSP_Naive.h"
#endif

@morganpackard
Copy link
Member

Supercollider uses something like this, that has been factored out into a
standalone library. We could take a look at that, but it could mean
breaking the "no dependencies" rule. Also could have license implications.

On Wed, Jan 28, 2015 at 3:55 PM, Nick Donaldson [email protected]
wrote:

Yes, this is a great idea. I had started down this road awhile back but
never saw it through. Another good option might be to have multiple .cpp
implementation files for the core DSP header that each use platform macros
to compile the whole thing or nothing.

i.e.

// TonicDSP_apple.cpp
#ifdef USE_APPLE_ACCELERATE
#include "TonicDSP.h"
...// All the implementations for the DSP functions
...
#endif

and so on.

Reply to this email directly or view it on GitHub
#253 (comment).

Morgan Packard
cell: (720) 891-0122
twitter: @morganpackard

@morganpackard
Copy link
Member

@andik
I meant that (as I understand it) the compiler may inline functions even if
they aren't marked inline. I'd vote for header-only. The function call will
have a tiny impact on performance, but I'd rather eliminate that tiny
impact if there's no strong reason not to.

On Wed, Jan 28, 2015 at 4:39 PM, andik [email protected] wrote:

@morganpackard https://github.com/morganpackard
I'm not a native english speaker so I'm not shure if I understood you
correctly. Did you mean that the compiler is free to not inline functions
marked with inline? I heard that this is possible. But in my working
place we inline a lot to have high performance and at least the DSP
forwarding functions are so small they definitively get inlined. I'm not
shure for the naive (for-loop) implementations. But there are so much
calls, it doesn't matter ^^
I'd especcially go for inline functions because they are easier to use by
the end-user and they raise better errors. Using macros was my first Idea,
but I'd prefer not to use them.

@ndonald2 https://github.com/ndonald2
this is a good idea, except that we'd loose inlining, which may be not
problem for the naive (for-loop) implementation of the functions.

So maybe we could use something like

// TonicDSP.h
#ifdef USE_APPLE_ACCELERATE
#include "TonicDSP_Apple.h"
...// other Systems
...
#else
#include "TonicDSP_Naive.h"
#endif

Reply to this email directly or view it on GitHub
#253 (comment).

Morgan Packard
cell: (720) 891-0122
twitter: @morganpackard

@ndonald2
Copy link
Member

@morganpackard I was wrong in ever worrying about the overhead, it's negligible. Certain wizard-like engineers at a particular audio company that I have connections with have shared certain information about whether or not inlining such functions is important. It's definitely not. Turns out the compiler tends to inline such tiny functions anyway when optimizing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants