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

Polyphonic MIDI synth example #178

Open
Dewb opened this issue Jun 7, 2013 · 33 comments
Open

Polyphonic MIDI synth example #178

Dewb opened this issue Jun 7, 2013 · 33 comments
Assignees

Comments

@Dewb
Copy link
Member

Dewb commented Jun 7, 2013

I plan to take a stab at this sometime. I'm thinking a basic extension of the StandaloneTonicDemo sample with RtMidi and code for a simple voice allocator added.

@ghost ghost assigned Dewb Jun 7, 2013
@ndonald2
Copy link
Member

ndonald2 commented Jun 7, 2013

Sounds cool!

If it makes sense to abstract the voice manager into its own BufferFiller subclass that could be pretty useful. Unfortunately there's not currently a way to copy a synth since we can't walk the generator graph, but you could probably template it and do it with a synth subclass.

@Dewb
Copy link
Member Author

Dewb commented Jun 7, 2013

I went ahead and whipped up the simplest possible strategy that occurred to me:
https://github.com/Dewb/Tonic/tree/midi/Demo/Standalone/PolyMIDIDemo

Comments on how to make this more idiomatic and useful are definitely welcome!

Issues:

  • It will eventually crash in ControlGenerator::_tick() inside of ControlMidiToFreq()::computeOutput(), probably because I'm doing something dumb. Part of my motivation here was to start understanding Tonic better from a user perspective, so this is a good learning opportunity.
  • When actively playing notes, CPU usage is around 4% for a simple 8-voice synth. When I stop playing, though, usage goes up to 13%! Any thoughts? Might be the fault of the RtMIDI or Windows event loop.
  • The Max poly~ object is smart enough to not even run DSP for subpatches that are not playing notes. Is that currently possible in Tonic? Would it work to add and remove sub-synths from the mixer as voices go active and inactive?
  • Is there a way to get a signal when an ADSR finishes its release cycle? This would be necessary for properly disabling DSP for a voice, but it would also be useful so I could prefer voices that were not still releasing over voices that were.

@ndonald2
Copy link
Member

ndonald2 commented Jun 7, 2013

Sweet!

A few comments

It will eventually crash in ControlGenerator::_tick() inside of ControlMidiToFreq()::computeOutput()

I believe this can be attributed to the way you are using ControlMidiToFreq in VoiceData. This line is not thread safe:

pVoice->frequency.input(ControlValue(note));

Currently, hot-swapping inputs to any ControlGenerator or Generator is not supported due to the fact that we don't have a good concurrency model for the synthesis graphs yet. If the voice is in the middle of processing its output and the existing input to ControlMidiToFreq is on the stack when you swap it out for a new one, it will crash.

To get this to work, you should hold on to the ControlValue that feeds ControlMidiToFreq and just update its value when the note changes.

We want to ideally avoid taking locks as much as possible in the buffer callback -- you may have noticed that generator-level mutexes were removed. They were eating up way to much CPU - ideally there would be only one mutex per SynthesisContext, and all generators in the graph have access to it, but I haven't figured that out yet. If you have any thoughts I'd love to chat sometime.

When actively playing notes, CPU usage is around 4% for a simple 8-voice synth. When I stop playing, though, usage goes up to 13%

Hmm... I've seen this before in other C++ synth implementations I've done, and it's usually due to subnormal numbers in the sample data somewhere. However, I'm not really sure where they'd be coming from - typically this will come up for floats that are repeatedly multiplied by something less than 1.

Have you tried running it through a profiler to see where the extra cycles are being used?

The Max poly~ object is smart enough to not even run DSP for subpatches that are not playing notes. Is that currently possible in Tonic? Would it work to add and remove sub-synths from the mixer as voices go active and inactive?

That sounds like it might work. Currently, disabling a particular input or portion of the graph isn't really supported because all generators operate under the assumption that they are being ticked at the audio or control rate. I have plans to add a "reset" method to the Generator base classes so that it will work to disable them while not active and reset the state when they become active again, but that's not in place yet. A lot of these things will depend on the ability to efficiently keep track of the inputs of each object in the graph, which we haven't solved yet (member variables don't provide a generic pattern, and keeping inputs in stdlib containers is way too slow for efficient processing - I'd like to pick your brain on that too).

Is there a way to get a signal when an ADSR finishes its release cycle?

Not yet - I have an idea on how to create multiple outputs from a single object, but I haven't gotten around to working on it yet.

@morganpackard
Copy link
Member

My comments inline

Sent from my iPhone

On Jun 7, 2013, at 6:26 PM, "Nick D." [email protected] wrote:

Sweet!

A few comments

It will eventually crash in ControlGenerator::_tick() inside of
ControlMidiToFreq()::computeOutput()

I believe this can be attributed to the way you are using ControlMidiToFreqin
VoiceData. This line is not thread safe:

pVoice->frequency.input(ControlValue(note));

Currently, hot-swapping inputs to any ControlGenerator or Generator is not
supported due to the fact that we don't have a good concurrency model for
the synthesis graphs yet. If the voice is in the middle of processing its
output and the existing input to ControlMidiToFreq is on the stack when you
swap it out for a new one, it will crash.

To get this to work, you should hold on to the ControlValue that feeds
ControlMidiToFreq and just update its value when the note changes.

We want to ideally avoid taking locks as much as possible in the buffer
callback -- you may have noticed that generator-level mutexes were removed.
They were eating up way to much CPU - ideally there would be only one mutex
per SynthesisContext, and all generators in the graph have access to it,
but I haven't figured that out yet. If you have any thoughts I'd love to
chat sometime.

When actively playing notes, CPU usage is around 4% for a simple 8-voice
synth. When I stop playing, though, usage goes up to 13%

Hmm... I've seen this before in other C++ synth implementations I've done,
and it's usually due to subnormal numbers in the sample data somewhere.
However, I'm not really sure where they'd be coming from - typically this
will come up for floats that are repeatedly multiplied by something less
than 1.

Have you tried running it through a profiler to see where the extra cycles
are being used?

The Max poly~ object is smart enough to not even run DSP for subpatches
that are not playing notes. Is that currently possible in Tonic? Would it
work to add and remove sub-synths from the mixer as voices go active and
inactive?

That sounds like it might work. Currently, disabling a particular input or
portion of the graph isn't really supported because all generators operate
under the assumption that they are being ticked at the audio or control
rate. I have plans to add a "reset" method to the Generator base classes so
that it will work to disable them while not active and reset the state when
they become active again, but that's not in place yet. A lot of these
things will depend on the ability to efficiently keep track of the inputs
of each object in the graph, which we haven't solved yet (member variables
don't provide a generic pattern, and keeping inputs in stdlib containers is
way too slow for efficient processing - I'd like to pick your brain on that
too).

Currently, controlgenerators rely on the tick of the audio generators to
advance. If each synth has its own metro, and we stop calling tick on
silent synths, the metros will get out of sync. I

Is there a way to get a signal when an ADSR finishes its release cycle?

Not yet - I have an idea on how to create multiple outputs from a single
object, but I haven't gotten around to working on it yet.

The way I'd do it is add a "getFinishedTrigger" method to adsr, which
returns a controlgenerator (controltrigger maybe?) you can query for "has
changed".


Reply to this email directly or view it on
GitHubhttps://github.com//issues/178#issuecomment-19136286
.

@Dewb
Copy link
Member Author

Dewb commented Jun 7, 2013

Aha, yeah, getting rid of the input frobbing fixed it, thanks! I switched to named parameters rather than holding on to the ControlGenerators, something I was meaning to do anyway.

Dewb added a commit to Dewb/Tonic that referenced this issue Jun 8, 2013
Dewb added a commit to Dewb/Tonic that referenced this issue Jun 8, 2013
@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Built OSX so I could use the Xcode profiler. On my MBP, the CPU usage is 12% when playing notes and 85% when idle!

I tried setting the flush denormal to zero flag. At first I thought it helped a small amount, but after a few runs the numbers didn't look statistically different than the runs without the flag.

Here's a snapshot of the profiler after running the program and doing nothing: (Flush flag was off.)

screen shot 2013-06-07 at 11 10 32 pm

Here's a snapshot after playing a few notes and then stopping for a very long time (which seems to produce higher CPU usage than if you never played any notes.) Looks like a lot of time is spent in Compressor_::computeSynthesisBlock(). (Flush flag was on this run.)

screen shot 2013-06-07 at 11 27 45 pm

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Here's a snapshot after playing a few notes and then stopping for a very long time (which seems to produce higher CPU usage than if you never played any notes.) Looks like a lot of time is spent in Compressor_::computeSynthesisBlock().

That sounds an awful lot to me like denormals are the culprit. The compressor's amplitude envelope release phase is a one-pole recursive filter tending toward zero, so it fits exactly the scenario I described. If no note has ever been played, the amplitude envelope is exactly zero. After playing a note, the envelope will go positive and tend toward zero until it drops to denormal range.

Not sure why the flush flag didn't work for you -- did you use _MM_SET_FLUSH_ZERO_MODE(_MM_FLUSH_ZERO_ON)? If so, be aware that it needs to be done on the audio thread, so I don't know of a good place to do it once and only once, independent of platform.

Just as a sanity check, to verify it's a denormal issue, try running the same test, but in FilterUtils.h make the following change to onePoleLPFTick

  inline void onePoleLPFTick( TonicFloat input, TonicFloat & output, TonicFloat coef){
    output = ((1.0f-coef) * input) + (coef * output);
    if (output < std::numeric_limits<float>::min()){
      output = 0;
    }
  }

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Obviously that is an inefficient solution but it should verify that it's denormals that are the issue.

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Turning off limiting inside each of the Synth voices and the outer Synth seems to have helped some; now the CPU percentages are around 6% (active) and 30% (idle.)

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

To turn the flag on I was using:

#include <fenv.h>
fesetenv(FE_DFL_DISABLE_SSE_DENORMS_ENV);

I'll try your suggestion in onePoleFPLTick().

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Turned all the limiting back on and tried your solution; not much change, back to 12%/85%.

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Hmm I will try to do some profiling and see if I can figure it out. I can't imagine what else it would be besides denormals, especially since I've had this exact problem before.

Dewb added a commit to Dewb/Tonic that referenced this issue Jun 8, 2013
@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Ah I did forget there are a ton of other places that might have denormal issues - recursive filters, delays with feedback, etc - so a way to flush them globally is pretty important.

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

I think I got it! Add this to the start of BufferFiller::fillBufferOfFloats():

      // flush denormals on this thread

#ifdef _MM_SET_FLUSH_ZERO_MODE
      _MM_SET_FLUSH_ZERO_MODE(_MM_FLUSH_ZERO_ON);
#endif

For my test setup this cut my CPU usage from early-stage idle ~5%, late stage idle ~10-15% down to consistent ~2-3% idle.

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Aha, I got it too. Apparently, to use fesetenv on OSX there's a pragma you have to set first. You ran your test with _MM_SET_FLUSH_ZERO_MODE on Windows, right?

#include <fenv.h>
#pragma STDC FENV_ACCESS ON
...

int renderCallback( void *outputBuffer, void *inputBuffer, unsigned int nBufferFrames,
        double streamTime, RtAudioStreamStatus status, void *userData )
{
    static bool flagSet = false;
    if (!flagSet) {
        fesetenv( FE_DFL_DISABLE_SSE_DENORMS_ENV );
        flagSet = true;
    }

    synth.fillBufferOfFloats((float*)outputBuffer, nBufferFrames, nChannels);
    return 0;
}

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Nope, I was testing on OSX. The _MM_SET_FLUSH_ZERO_MODE macro is part of the xmm intrinsics header <xmmintrin.h> so in theory it should be available on any platform where SSE is supported. Repeated calls don't seem to incur any noticeable overhead so putting it in the bufferfiller fill function seems appropriate, that way it (ideally) will work for anyone using Tonic anywhere - not just the standalone demo, as you have it here.

I'm not sure what is more widely compatible between fesetenv() and the intrinsics macro - do you know? I don't mind either way, but I definitely think it should live internally to Tonic, and BufferFiller seems like a good place.

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Yeah, it should definitely be inside Tonic. std::fesetenv has been added to the C++ library as of C++11. Will the _MM macro work on iOS? No SSE on ARM, right?

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

I'm not 100% sure, but that's why I wrapped it in the #ifdef. If it's not there, it won't be compiled.

I want to say the ARM FPU already rounds denormals as the default, since the systems with which it's used are typically less concerned with precision and more with efficiency. This thread seems to sort of verify that:

http://stackoverflow.com/questions/7346521/subnormal-ieee-754-floating-point-numbers-support-on-ios-arm-devices-iphone-4

For now I'm going to push the macro-based fix to the development branch. Tomorrow I will do some on-device iOS profiling to see if it's an issue there.

Yet another great bug find. Thanks!

Dewb added a commit to Dewb/Tonic that referenced this issue Jun 8, 2013
@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Yep looks like it won't compile on-device for iOS with the xmmintrin header. I'll make the denormal flushing conditional to the build environment.

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Pushed to development. I'm not sure of any other non-SSE platforms that will need denormal rounding enabled, so for now it's defined like this:

#ifdef __SSE__
  #include <xmmintrin.h>
  #define  TONIC_ENABLE_DENORMAL_ROUNDING() _MM_SET_FLUSH_ZERO_MODE(_MM_FLUSH_ZERO_ON) 
#else
  #define  TONIC_ENABLE_DENORMAL_ROUNDING()
#endif

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

MSVS apparently doesn't define __SSE__, so this returns denormals to the Windows build.

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Hmm... let me boot to windows and check it out.

On Sat, Jun 8, 2013 at 1:37 PM, Michael Dewberry
[email protected]:

MSVS apparently doesn't define SSE, so this returns denormals to the
Windows build.


Reply to this email directly or view it on GitHubhttps://github.com//issues/178#issuecomment-19152430
.

Nick Donaldson
[email protected]

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

This works:

#if defined(__SSE__) || defined(_WIN32)
  #include <xmmintrin.h>
  #define  TONIC_ENABLE_DENORMAL_ROUNDING() _MM_SET_FLUSH_ZERO_MODE(_MM_FLUSH_ZERO_ON) 
#else
  #define  TONIC_ENABLE_DENORMAL_ROUNDING()
#endif

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Cool, I'll add that.

On Sat, Jun 8, 2013 at 1:39 PM, Michael Dewberry
[email protected]:

This works:

#if defined(SSE) || defined(_WIN32)
#include <xmmintrin.h>
#define TONIC_ENABLE_DENORMAL_ROUNDING() _MM_SET_FLUSH_ZERO_MODE(_MM_FLUSH_ZERO_ON)
#else
#define TONIC_ENABLE_DENORMAL_ROUNDING()
#endif


Reply to this email directly or view it on GitHubhttps://github.com//issues/178#issuecomment-19152455
.

Nick Donaldson
[email protected]

@Dewb
Copy link
Member Author

Dewb commented Jun 8, 2013

Should the macro definition go in TonicCore.h? Even if it's never called anywhere but BufferFiller, that's where all the other platform-specific code is.

@ndonald2
Copy link
Member

ndonald2 commented Jun 8, 2013

Good call.

On Sat, Jun 8, 2013 at 1:45 PM, Michael Dewberry
[email protected]:

Should the macro definition go in TonicCore.h? Even if it's never called
anywhere but BufferFiller, that's where all the other platform-specific
code is.


Reply to this email directly or view it on GitHubhttps://github.com//issues/178#issuecomment-19152550
.

Nick Donaldson
[email protected]

@morganpackard
Copy link
Member

You guys are amazing.

On Sat, Jun 8, 2013 at 1:45 PM, Nick D. [email protected] wrote:

Good call.

On Sat, Jun 8, 2013 at 1:45 PM, Michael Dewberry
[email protected]:

Should the macro definition go in TonicCore.h? Even if it's never called
anywhere but BufferFiller, that's where all the other platform-specific
code is.


Reply to this email directly or view it on GitHub<
https://github.com/TonicAudio/Tonic/issues/178#issuecomment-19152550>
.

Nick Donaldson
[email protected]

Reply to this email directly or view it on GitHubhttps://github.com//issues/178#issuecomment-19152556
.

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

@Dewb
Copy link
Member Author

Dewb commented Jun 14, 2013

FYI, I think my next step here is to put this together with JUCE and make a VST demo.

@kphillisjr
Copy link

I saw this issue, and was wondering what the current status is. I did not see a lot of changes on the tree.

@kallaballa
Copy link
Contributor

I'm currently working on reviving the project and it probably will take a while before things get in motion. Please check out #282

@kallaballa
Copy link
Contributor

oh i forgot... there is an example for a polyphonic synth, but it isn't a minimal one.
https://github.com/kallaballa/MidiPatch

@Dewb
Copy link
Member Author

Dewb commented May 13, 2020

This didn't get PR'd to master at the time because the demos folder was being reorganized and the approach to building cross-platform was changing. It would be fairly straightforward to update and PR this branch for PC/Mac (iOS is likely more complicated.)

@kallaballa
Copy link
Contributor

Alright. I'll look into it. But first i gotta get around to making a release.

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

5 participants