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

review partial work on issue 70 #79

Merged
merged 14 commits into from
Jan 12, 2016
Merged

review partial work on issue 70 #79

merged 14 commits into from
Jan 12, 2016

Conversation

nwolek
Copy link
Member

@nwolek nwolek commented Jan 3, 2016

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
Copy link
Member

tap commented Jan 3, 2016

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
Copy link
Member Author

nwolek commented Jan 3, 2016

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
Copy link
Member Author

nwolek commented Jan 4, 2016

@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
Copy link
Member Author

nwolek commented Jan 4, 2016

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

@nwolek
Copy link
Member Author

nwolek commented Jan 12, 2016

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

nwolek added a commit that referenced this pull request Jan 12, 2016
review partial work on issue 70
@nwolek nwolek merged commit 078350a into master Jan 12, 2016
@nwolek nwolek deleted the issue/70a branch January 12, 2016 14:11
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

Successfully merging this pull request may close these issues.

2 participants