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

YIN algorithm, conversion of GuitarTuner example to Swift 3 #39

Merged
merged 10 commits into from Oct 23, 2016

Conversation

glaurent
Copy link
Contributor

The GuitarTuner example has also been changed to use the YIN algorithm, which, from the testing I've done (electric and acoustic guitar), gives better results than HPS. The correct note is found on the whole range of the instrument.

Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Poor results so far :-(

Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Apparently the problem is the YINUtil:difference() function. The simpler (but slower) difference2() gives good results. Setting the threshold to an appropriate value seems to have helped too.

Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
…consuming

Instruments shows that the difference2() function eats more than 50% of CPU. I need to find a faster way to do this, and I can’t seem to get the difference() function working.

Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Drops CPU usage from 95% to 25%.

Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Signed-off-by: Guillaume Laurent <glaurent@telegraph-road.org>
Copy link
Owner

@vadymmarkov vadymmarkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glaurent This is just amazing! I will test it with my guitars and probably make a little cleanup to keep code base more consistent. It also deserves to be in REAME, so I'll update it as well. Thanks for your investment and contribution!


// slow and eats a lot of CPU, but working
//
class func difference2(buffer:[Float]) -> [Float] {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glaurent Is there any reason of having class functions here? Or if it's meant to be a set of functions, could be a struct with static func, right?

@PCrompton
Copy link

@glaurent @vadymmarkov Thanks for all your work on this! I've been caught up in other projects right now but I'm excited to get back to my project involving this framework. I'd love to contribute to this project at some point but I need to better learn the concepts behind the code. Do you recommend any books or other resources that will help me understand how to parse sound waves? I have a music degree but aspire to get into the software development field professionally, and my dream is to help develop software for musicians and music education. Thanks again!

@glaurent
Copy link
Contributor Author

glaurent commented Oct 23, 2016

@vadymmarkov You're welcome. Thank you for your work on Beethoven, it has helped me immensely too.

As for your question, yes this could be a struct with static func, I'm not sure how much of a difference that would make as far as the language is concerned. I wrote it this way because the original C++ code is written that wayl, and it feels somewhat more natural to me too. I noticed you seem to prefer structs to classes, though :)

BTW, is there a reason to have the TransformStrategy user-settable in the Config class ? As far as I can see, the EstimationStrategy implies a TransformStrategy, so setting it should suffice, and from it the Config would set the TransformStrategy...

@glaurent
Copy link
Contributor Author

@PCrompton To be honest, what you ask is a bit like asking how to play a Jazz solo having hardly ever touched an instrument. I only have vague recollection of basic signal processing classes from college, I could implement this algorithm only because I had two sample implementations that I could use as reference, and even then there was a whole lot of guesswork. Computing pitch information from a soundwave is actually still a research topic, and pretty hard to do, there's a lot of complex math involved. Get the basics of iOS development first, then you might want to tackle this kind of problems.

@vadymmarkov
Copy link
Owner

vadymmarkov commented Oct 23, 2016

@PCrompton If you are familiar with iOS/Mac development basics, you can read about Core audio and AVFoundation to understand the concepts of working with audio. As for pitch detection, you can start with Wikipedia, there is a list of references that could possibly put you into the right direction. But it's a research topic, as @glaurent said, so normally I'm trying to discover the particular problem by reading different kinds of resources and/or exploring existing implementations in other programming languages.

@vadymmarkov
Copy link
Owner

@glaurent Yeah, I tend to use structs whenever it's not necessary to have a reference type 😄 I agree with you that the transform strategy is probably redundant in Config. At first I though it could be more flexible, but it seems like transformation and estimation go together anyway, so it might be something that I'd like to refactor.

@vadymmarkov
Copy link
Owner

vadymmarkov commented Oct 23, 2016

I'm going to merge this PR. Will do some refactoring, as I mentioned before, and will make a new release straight after that. Great job @glaurent !

@vadymmarkov vadymmarkov merged commit 0192a7d into vadymmarkov:master Oct 23, 2016
@glaurent
Copy link
Contributor Author

@vadymmarkov I tend to start from the opposite viewpoint (which AFAIK is more common) : you use classes unless it's necessary or convenient to have a value type. Anyway in this case it doesn't make much of a difference.

Thanks for the merge, you're welcome :). I'll let you close the pullreq.

@glaurent glaurent deleted the YINalgorithm branch October 23, 2016 16:01
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.

None yet

3 participants