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

Chroma shifted when using more than 12 bands #391

Open
harrislapiroff opened this issue May 6, 2020 · 11 comments
Open

Chroma shifted when using more than 12 bands #391

harrislapiroff opened this issue May 6, 2020 · 11 comments

Comments

@harrislapiroff
Copy link

When using 12 bands for chroma extraction (the default configuration) C is the zeroth band. When using more than 12, C seems to be shifted up to be a higher band.

Steps to reproduce:

  1. Run a chroma band extraction on an audio buffer array of Middle-C with 24 bands:
  Meyda.chromaBands = 24
  const chroma = Meyda.extract('chroma', audioBuffer.getChannelData(0).slice(0, 4096))

Expected result: Value of band 0 should be 1, all other bands < 1
Actual result: Value of band 3 is 1, all other bands < 1

See interactive reproduction here (best in Firefox).

@hughrawlinson
Copy link
Member

Hey, thanks for filing the issue. We'll look into it as soon as we can :)

@harrislapiroff
Copy link
Author

Thank you! Happy to help out any way I can, though my knowledge of audio analysis is pretty scant.

@harrislapiroff
Copy link
Author

Updating here to say that as far as I can tell this bug still exists and I'm fairly sure the transposition error is happening somewhere in this function, but I lack the theoretical understanding to parse out where

export function createChromaFilterBank(

@hughrawlinson
Copy link
Member

Hey @harrislapiroff, sorry it's been over a year. I'm not really sure why this is happening. @odub I wonder if you have any ideas?

@odub
Copy link

odub commented Sep 8, 2021

@harrislapiroff thanks for the beautifully documented example. Some cases like that would be valuable in the test suite if you have more appetite to work on this :)

I can't pick up the implementation on the fix but here's a driveby for one thing I see going wrong that might help you.

This code to make the output data C-based rather than A-based won't work for any number of chroma bands other than 12.

Its job is to shift the bottom three bins of the output data to the end of the array assuming that the bins correspond to semitones (3 semitones from A to C). Where that's not the case we're at sea.

Scaling that number based on the number of chroma bands might be a way forward towards supporting other numbers of divisions correctly. For the 24-band case, it looks like shifting by 6 rather than by 3 would get you a more expected output.

@odub
Copy link

odub commented Sep 8, 2021

I can't guarantee there won't be other issues even where that one is fixed, but that's one I see for now anyway

@hughrawlinson
Copy link
Member

Thanks for looking into it @odub! That makes sense. I'll look into patching this.

@harrislapiroff
Copy link
Author

Just glancing at this issue again and noticed my example case has slightly different wrong behavior now in my browser. It looks like when extracting to 24 bands middle C is indeed now at the 0th band, but when extracting to 12 bands it's the 10th(?)

Was there a deliberate behavior change to chroma extraction in the intervening years, out of curiosity?

@hughrawlinson
Copy link
Member

Uh, not intentionally! 😬 I wonder if you know what version you were using originally?

@harrislapiroff
Copy link
Author

Hmm looking at it again I'm now seeing the same wrong behavior I expected. Not sure what was going on there. I added a dropdown to my notebook to switch versions though, which should help with any debugging.

@hughrawlinson
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants