jamoma / jamoma2

A header-only C++ library for building dynamic and reflexive systems with an emphasis on audio and media.
MIT License
30 stars 6 forks source link

review partial work on issue 70 #79

Closed nwolek closed 8 years ago

nwolek commented 8 years ago

I would not consider this done (hence the 70a branch name), but I would like @tap to give this a look and comment on the direction it is going. Not sure I like the use of so many try-catch statements to keep things in bounds. If you have any other ideas, I would appreciate them!

tap commented 8 years ago

Thanks for this, @nwolek .

In performance-sensitive code we cannot use exceptions. So your intuition that something is amiss using the try-catches here is correct.

Actually, speaking of performance-sensitive audio code there is, IMO, a really good video @ https://www.youtube.com/watch?v=boPEO2auJj4 (YMMV)

( @lossius may also appreciate the video talk )

A couple of other observations:

h3. Arg Order

Sample at(size_t channel, double interpolatedFrameIndex)

I would suggest reversing the arguments so that interpolatedFrameIndex is first. If we make channelIndex second then it can default to the first channel so you don't have to provide it unless you need a specific channel. e.g.

Sample at(double interpolatedFrameIndex, size_t channelIndex = 0)

I also I changed the name here because we aren't passing the whole channel, but the index number of the channel.

h3. Template specification

As I'm sure you are already thinking, it's bummer to have to write code like this:

temp = test_bundle.at<Jamoma::Interpolator::Cosine<Sample>>(0,d);

instead of

temp = test_bundle.at(0,d);

As I think about it though, it would be less desirable to make the interpolation a part of the state of the SampleBundle. So I guess that means we should experiment with just making this line of code a bit less long. Finding some shorthand for it can be an issue assigned to me. As long as this is functional though we can keep moving forward even if it is a lot of typing to make those statements.

nwolek commented 8 years ago

Video

That was a good one! I think he did a good job surveying the issues I knew, and helped me better understand the issues I am less familiar with.

Arg order

Good idea putting channel last. I was thinking that we might eventually have other versions that provide you with all channels in a single SampleBundle. I would also like something where we pass in a vector of interpolation indices and get back a SampleBundle. Something like:

SampleBundle at(double interpolatedFrameIndex) {
    // returns all channels at a specific index
}

SampleBundle at(std::vector<double> interpolatedFrameIndices, size_t channelIndex = 0) {
    // returns a SampleBundle with a size that matches interpolatedFrameIndices.size()
    // filled with interpolated values from the channel at channelIndex
}

Template specs

Agreed. Part of my reason for writing it out was to show just how ugly it was.

nwolek commented 8 years ago

@tap - Good call removing the try-catch statements. Timing the interpolation of about 500 samples was about 75% faster when replaced by good old if statements.

nwolek commented 8 years ago

@tap - please give this another look and let me know if you think this is ready to merge into master.

nwolek commented 8 years ago

I believe I have implemented all that @tap outlined 9 days ago, so I am moving forward with merge.