Windows port: Visual Studio projects for TonicLib and StandaloneTonicDemo

[Dewb]

Release build works; there's an odd stack-overwriting crash in the Debug build which is worth looking into further, since it seems to be triggered by the debug runtime's memory guards.

[ndonald2]

@Dewb @morganpackard One thing to examine for the bug in the Windows debug build would be the way the >> operator is overloaded in Effect.h:

  template<class EffectType, class EffectType_>
  static EffectType & operator>>(Generator lhs, TemplatedEffect<EffectType, EffectType_> rhs){
    return rhs.input( lhs );
  }

There's some stuff about this that always felt unclean to me, not sure if it could be related to the bug. It's a template on top of a somewhat recursive template, all so Effect subclasses inherit the "input" method and can still return a reference cast to the correct type, so setters can be strung together. Also it's returning a reference to the right hand side, which also feels weird.

@Dewb Your kung-fu is better than my kung-fu... does any of this seem like it might spell bad news?

A quick test to see if it's still behaving strangely would be not to use the >> operator but instad the input setters.

[Dewb]

Yeah, this is almost certainly the problem. Using .input() directly avoids the crash. I think one of your copy-constructed temporaries is being used after it's been popped off the stack, but you get lucky in a release build, and it takes the 0xCCCC stack wiping in the debug CRT to catch it. I don't quite see where the temporary is being used yet, but I'm going to stare at it until it comes to me.

[Dewb]

Ah, it was right in front of my nose. Making the rhs argument a reference fixes the problem, like so:

template<class EffectType, class EffectType_>
static EffectType & operator>>(Generator lhs, TemplatedEffect<EffectType, EffectType_>& rhs){
    return rhs.input( lhs );
}

When rhs is not a reference, it's a stack temporary created via the copy constructor. You're calling .input on it and returning another ref to it, but that doesn't keep it from being popped off the stack when the operator>> function epilogue executes. The copy constructor that was crashing is the one trying to assign the result of foo >> bar to output, and rhs here needs to stay alive long enough to be copied again.

This also works:

template<class EffectType, class EffectType_>
static EffectType operator>>(Generator lhs, TemplatedEffect<EffectType, EffectType_> rhs){
  return rhs.input( lhs );
}

because when the return value is not a reference, rhs will be copy-constructed again into said return value before rhs is popped.

I don't know which direction makes more sense for your design. You can construct a copy of an argument, or you can return a reference as a reference, but you can't have a function return a reference to a non-reference argument.

[Dewb]

(My suspicion is that all-references is better, because you call the copy constructor two fewer times, but it's not like it's expensive.)

[morganpackard]

I personally think references are the lamest feature included in c++. So confusing to me, again and again.

But as long as the bug is fixed (hooray!), I don't think it's a big deal one way or the other.

Sent from my iPhone

On May 21, 2013, at 12:34 AM, Michael Dewberry notifications@github.com wrote:

(My suspicion is that all-references is better, because you call the copy constructor two fewer times, but it's not like it's expensive.)

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18188339 .

[ndonald2]

gasp such heresy! :)

Dewb you rock. I am glad it was something small. I think references are best practice for operator overloading, right? So let's stick with that.

Thank you!

On Tuesday, May 21, 2013, Morgan Packard wrote:

I personally think references are the lamest feature included in c++. So confusing to me, again and again.

But as long as the bug is fixed (hooray!), I don't think it's a big deal one way or the other.

Sent from my iPhone

On May 21, 2013, at 12:34 AM, Michael Dewberry <notifications@github.com<javascript:_e({}, 'cvml', 'notifications@github.com');>>

wrote:

(My suspicion is that all-references is better, because you call the copy constructor two fewer times, but it's not like it's expensive.)

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18188339

.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18206036 .

Nick Donaldson ndonald2@gmail.com

[Dewb]

I made the arguments for both Effect and ControlConditioner operator>> references. At some point I think you might want to make your operator +-*/ args references too.

There's still a piece missing from my explanation of the problem. I tried to create the smallest possible example program that demonstrated the crash, but I can't do it with a non-reference operator>> alone. Something about the pattern of layered copy constructors here makes the reference omission more serious. I don't think there's anything further to worry about, just academic interest.

[ndonald2]

Hmm... very strange indeed. We're passing around our smart pointer objects by value pretty much everywhere so I wonder if that's at risk, too (I really hope not). One of the whole benefits of the smart pointer is that in theory it should be OK to pass them around by value.

I wonder if it's an artifact of the way the compiler deals with operator overloading vs normal functions.

[Dewb]

I was about to say that this PR is ready to be merged if you guys were happy with it, but... there's another problem with the Windows debug build!

In the standalone demo, everything sounds fine for a few seconds, but once the filter freq drops and the volume gets low, the output just turns into clicks. It comes back later, but then drifts in and out of click-land. Release build doesn't have this issue.

[ndonald2]

Hmmm... not sure what to make of the clicks. My initial thought is that somewhere in the DSP chain there are subnormal numbers when the volume gets low, so your CPU usage spikes a bit and can't keep up with the buffers. Haven't had to deal with that explicitly in OSX but it's something that can happen.

Keep an eye on your CPU usage when you start hearing the clicks, if it spikes, that might be it. Another way to verify would be to make your buffer size much larger (4096 or so) and see if you still hear the clicks.

Are you using DS or ASIO? DS is notoriously awful in terms of latency so it might barely be getting by at the current buffer size.

[Dewb]

Increasing buffer size to 4096 doesn't help. CPU is constant around 1.2-1.4%. Yep, I'm using DS.

On Tue, May 21, 2013 at 11:46 AM, Nick D. notifications@github.com wrote:

Hmmm... not sure what to make of the clicks. My initial thought is that somewhere in the DSP chain there are subnormal numbers when the volume gets low, so your CPU usage spikes a bit and can't keep up with the buffers. Haven't had to deal with that explicitly in OSX but it's something that can happen.

Keep an eye on your CPU usage when you start hearing the clicks, if it spikes, that might be it. Another way to verify would be to make your buffer size much larger (4096 or so) and see if you still hear the clicks.

Are you using DS or ASIO? DS is notoriously awful in terms of latency so it might barely be getting by at the current buffer size.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18217834 .

[ndonald2]

My other thought is just that it's clipping, but that shouldn't happen with the limiter on each synth by default. Also strange that it's only on debug.

[Dewb]

Issue goes away if I remove the SineWaves from the delay time.

On Tue, May 21, 2013 at 12:25 PM, Nick D. notifications@github.com wrote:

My other thought is just that it's clipping, but that shouldn't happen with the limiter on each synth by default. Also strange that it's only on debug.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18219918 .

[ndonald2]

That's a good clue. There may be some issue with the delay line if the delay time is modulating.

[Dewb]

If I leave the sines on the delay time but set the feedback to zero, it still clicks but far less often, just once per perceptual cycle of the audio.

[ndonald2]

I'll take a look at it. If I can't figure it out I might just have to get VS so I can repro the issue.

[Dewb]

If you weren't already aware, VS 2012 Express Desktop is free. It's what I've been using. http://www.microsoft.com/visualstudio/eng/products/visual-studio-express-for-windows-desktop

On Tue, May 21, 2013 at 1:23 PM, Nick D. notifications@github.com wrote:

I'll take a look at it. If I can't figure it out I might just have to get VS so I can repro the issue.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18223538 .

[ndonald2]

Awesome, thanks. I don't have easy access to a Windows machine at work but I will take a look sometime this week when I'm home.

[ndonald2]

I got Visual Studio last night and I was able to figured out the clicking thing.

In rare situations the delay line can index out of bounds. If the floating point "read head" index is less than zero, it has the total delay line length added to it to wrap back within the correct range. However if it's just barely less than zero (-0.000512 or something), wrapping back rounds it up to the full delay line length (length of the array), which is out of bounds by 1.

I added a check to make sure that index is still in bounds when we go to read the interpolated value from the delay line, and it fixed the problem. I'll add it to our master and you can merge it in.

Nice catch! Probably wasn't an issue on our builds or in your Release build because the out-of-bounds memory was zeroed.

BTW if you're interested in why it was "clicking" - the out-of-bounds value read from the delay line was almost always a very large number (10^28 or so), which causes the peak limiter's gain envelope to skyrocket, so the compensation attenuation was HUGE, and you get basically silence, punctuated by small clicks where there was a huge number in the output. That's how I was able to trace it back. The output from the delay effect seemed to look fine while it was silent, but the final output was super tiny.

[ndonald2]

OK, fix pushed to master.

What an interesting bug! The feedback having an effect on it was due to the huge sample getting pushed through the delay line again and again, shrinking a bit each time. If the feedback was zero, it made its way through only once, so the silence only persisted for as long as the limiter's release time took to return the attenuation back to a reasonable level.

[Dewb]

That is indeed an interesting bug! Thanks for the explanation. It's like they always say, there are just two hard problems in computer science: cache invalidation, naming things, and off-by-one errors.

Memory guards in debug builds really do help shake subtle bugs out of the bushes. I tried turning on the Xcode memory guards, which I have less experience with, and got some crashes right away; I'll look into those after the Windows port is done.

[ndonald2]

Yeah, it's probably a good idea to go through with guard malloc and fix any outstanding memory issues. I noticed one with some of the filters, I'll check that one out.

[ndonald2]

@morganpackard @Dewb how are we feeling about getting this merged in? Seems pretty stable now.

[morganpackard]

Are we in a hurry? I'd rather let more people bang on it before we merge it in to the master than merge it in with bugs we're not aware of. Maybe it's time to talk about release process. Make a pre-release branch and merge it into that maybe? It's not such a big deal one way or another at this point, but I like the idea of establishing good habits and process early.

On Fri, May 24, 2013 at 2:48 PM, Nick D. notifications@github.com wrote:

@morganpackard https://github.com/morganpackard @Dewbhttps://github.com/Dewbhow are we feeling about getting this merged in? Seems pretty stable now.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18422352 .

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

[morganpackard]

It could be nice to be able to send an email to everybody who gave us there address at mmmmmaven, and say "we want to merge this windows project, can you all test it".

On Fri, May 24, 2013 at 3:02 PM, Morgan Packard morgan@morganpackard.comwrote:

Are we in a hurry? I'd rather let more people bang on it before we merge it in to the master than merge it in with bugs we're not aware of. Maybe it's time to talk about release process. Make a pre-release branch and merge it into that maybe? It's not such a big deal one way or another at this point, but I like the idea of establishing good habits and process early.

On Fri, May 24, 2013 at 2:48 PM, Nick D. notifications@github.com wrote:

@morganpackard https://github.com/morganpackard @Dewbhttps://github.com/Dewbhow are we feeling about getting this merged in? Seems pretty stable now.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18422352 .

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

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

[ndonald2]

I definitely like the idea of a pre-release branch, that's something we should establish right away.

It could be nice to be able to send an email to everybody who gave us there address at mmmmmaven, and say "we want to merge this windows project, can you all test it".

I think that's maybe an idea to consider for this particular case since we didn't have Windows support at all before, but in general I think that kind of testing strategy only works if we know that we can rely on end-users to regularly and promptly test pre-release branches (or setting up a separate list for people who want to beta test). The problem with crowdsourcing QA is that not everyone who uses the library has the time or interest to pull down a pre-release branch and test new changes (and getting emails like that every time we have new features to merge in could become annoying to a lot of people). Perhaps we should first just gauge interest among those people in receiving updates about beta features and/or becoming regular beta testers.

Since this is a small operation that's 100% open source, I actually think a good way crowdsource QA on a project like this is to reach an agreement among the core contributors that a pre-release version is stable, push it to master, and then just let people find any bugs we missed in the master branch. At the end of the day, if it's stable in the build configurations under which we are developing it (and the configurations we define as being fully-supported), chances are it's going to be stable for the majority of users. It's easy to get caught up in trying to make sure it works on every possible configuration, whereas one of the great things about having an open-source project is that people with significantly different configurations can report issues or submit pull requests to us if it doesn't work for them.

With that in mind we should most definitely establish well-defined test practices among ourselves. For starters, that might include:

• Defining the core supported platforms/configurations for which we are testing
• All demos build and run stably on these core platforms
• All unit tests pass

Whatever the final checklist is, it should be checked off before submitting or verifying a pull request.

I'm happy to have an offline chat about this sometime, too, just wanted to get my thoughts down.

[Dewb]

I've been looking into what it would take to run your unit tests on the Windows build; I think that's a necessary task before release. The trouble is that Microsoft doesn't include a unit testing framework in the free version of Visual Studio, so we'll either need to add a dependency on an external tool, or roll our own test harness (which could be as simple as a batch file.)

On Fri, May 24, 2013 at 4:21 PM, Nick D. notifications@github.com wrote:

I definitely like the idea of a pre-release branch, that's something we should establish right away.

It could be nice to be able to send an email to everybody who gave us there address at mmmmmaven, and say "we want to merge this windows project, can you all test it".

I think that's maybe an idea to consider for this particular case since we didn't have Windows support at all before, but in general I think that kind of testing strategy only works if we know that we can rely on end-users to regularly and promptly test pre-release branches (or setting up a separate list for people who want to beta test). The problem with crowdsourcing QA is that not everyone who uses the library has the time or interest to pull down a pre-release branch and test new changes (and getting emails like that every time we have new features to merge in could become annoying to a lot of people). Perhaps we should first just gauge interest among those people in receiving updates about beta features and/or becoming regular beta testers.

Since this is a small operation that's 100% open source, I actually think a good way crowdsource QA on a project like this is to reach an agreement among the core contributors that a pre-release version is stable, push it to master, and then just let people find any bugs we missed in the master branch. At the end of the day, if it's stable in the build configurations under which we are developing it (and the configurations we define as being fully-supported), chances are it's going to be stable for the majority of users. It's easy to get caught up in trying to make sure it works on every possible configuration, whereas one of the great things about having an open-source project is that people with significantly different configurations can report issues to us if it doesn't work for them.

With that in mind we should most definitely establish well-defined test practices among ourselves. For starters, that might include:

• Defining the core supported platforms/configurations for which we are testing
• All demos build and run stably on these core platforms
• All unit tests pass

Whatever the final checklist is, it should be checked off before submitting or verifying a pull request.

I'm happy to have an offline chat about this sometime, too, just wanted to get my thoughts down.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18427714 .

[morganpackard]

Nick- don't sweat the little bugs too much--I like that. Open source framework. Work in progress, continually. You got my vote if you want to merge.

Mike-- regarding unit testing, dependencies for running tests don't bother me so much. Just don't want any for running the core tonic stuff.

Sent from my iPhone

On May 24, 2013, at 5:49 PM, Michael Dewberry notifications@github.com wrote:

I've been looking into what it would take to run your unit tests on the Windows build; I think that's a necessary task before release. The trouble is that Microsoft doesn't include a unit testing framework in the free version of Visual Studio, so we'll either need to add a dependency on an external tool, or roll our own test harness (which could be as simple as a batch file.)

On Fri, May 24, 2013 at 4:21 PM, Nick D. notifications@github.com wrote:

I definitely like the idea of a pre-release branch, that's something we should establish right away.

It could be nice to be able to send an email to everybody who gave us there address at mmmmmaven, and say "we want to merge this windows project, can you all test it".

I think that's maybe an idea to consider for this particular case since we didn't have Windows support at all before, but in general I think that kind of testing strategy only works if we know that we can rely on end-users to regularly and promptly test pre-release branches (or setting up a separate list for people who want to beta test). The problem with crowdsourcing QA is that not everyone who uses the library has the time or interest to pull down a pre-release branch and test new changes (and getting emails like that every time we have new features to merge in could become annoying to a lot of people). Perhaps we should first just gauge interest among those people in receiving updates about beta features and/or becoming regular beta testers.

Since this is a small operation that's 100% open source, I actually think a good way crowdsource QA on a project like this is to reach an agreement among the core contributors that a pre-release version is stable, push it to master, and then just let people find any bugs we missed in the master branch. At the end of the day, if it's stable in the build configurations under which we are developing it (and the configurations we define as being fully-supported), chances are it's going to be stable for the majority of users. It's easy to get caught up in trying to make sure it works on every possible configuration, whereas one of the great things about having an open-source project is that people with significantly different configurations can report issues to us if it doesn't work for them.

With that in mind we should most definitely establish well-defined test practices among ourselves. For starters, that might include:

• Defining the core supported platforms/configurations for which we are testing
• All demos build and run stably on these core platforms
• All unit tests pass

Whatever the final checklist is, it should be checked off before submitting or verifying a pull request.

I'm happy to have an offline chat about this sometime, too, just wanted to get my thoughts down.

— Reply to this email directly or view it on GitHub< https://github.com/TonicAudio/Tonic/pull/157#issuecomment-18427714> .

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18431928 .

[ndonald2]

I think we should at least get some sort of unit testing in place for Windows before we make it official. I'm gonna setup a pre-release branch (I will just call it "development") so let's open pull requests from feature branches against that from now on.

@Dewb Since dependencies aren't an issue, how much effort would it be to create a separate VS project with a very basic unit testing framework for Windows? Whatever the simplest option is that can execute setup->test->teardown for a bunch of test cases and provides an easy way to assert and log results.

@morganpackard That's why I love grassroots open source libraries - your end users are your testers, and often your peers. It's a great way to collaborate with strangers.

[Dewb]

Oh, if you don't mind tool dependencies, then it wouldn't be much work to use Google Test to do C++ testing on Windows/Linux. https://code.google.com/p/googletest/

On Fri, May 24, 2013 at 9:32 PM, Nick D. notifications@github.com wrote:

I think we should at least get some sort of unit testing in place for Windows before we make it official. I'm gonna setup a pre-release branch (I will just call it "development") so let's open pull requests from feature branches against that from now on.

@Dewb https://github.com/Dewb Since dependencies aren't an issue, how much effort would it be to create a separate VS project with a very basic unit testing framework for Windows? Whatever the simplest option is that can execute setup->test->teardown for a bunch of test cases and provides an easy way to assert and log results.

@morganpackard https://github.com/morganpackard That's why I love grassroots open source libraries - your end users are your testers, and often your peers. It's a great way to collaborate with strangers.

— Reply to this email directly or view it on GitHubhttps://github.com/TonicAudio/Tonic/pull/157#issuecomment-18437767 .

[ndonald2]

@Dewb Lack of unit tests notwithstanding, this is looking pretty good to me. I didn't have any issues on Windows after the bugfix.

I'd be pretty confident in merging this to development since we've decided to use that branch as our staging area for major changes. Would you mind updating your fork, merging your changes here into your own development branch, making any necessary updates to get it working, and re-opening the pull request?

No big rush. I believe @morganpackard and I are both now able to test on Windows so we can also proof things in the development branch, and we can add unit tests later. It might end up being easier for all platforms to use the same set of unit tests anyway.

Also, I wanted to ask @morganpackard about possibly making you a collaborator. No pressure or commitment involved, it just might make it easier for you to contribute windows-related stuff going forward, if that's something you're interested in doing.

[Dewb]

Closing now that there's a PR on the development branch.