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

use full decimation instead of fast #133

Closed
wants to merge 1 commit into from
Closed

Conversation

egitto
Copy link

@egitto egitto commented Jan 23, 2020

image
Figure 1. Top-right: the fast decimation function. Bottom-left: the slow decimation function. Note the prominence of the artifacts generated by higher harmonics.

TL;DR

This PR switches the decimation function from the fast version to the slower, better version.

Costs and Benefits

The benefit of this is that a plotted spectrum is more accurate (though still not perfectly) in the presence of frequencies higher than the maximum frequency in the view, such as harmonics.

The cost of this is that it makes the decimation function take almost as much time as the FFT function (93%) instead of much less (7%). (caveat: I don't know if the FFT is actually the proper thing to compare it to, it was just convenient)

I think this is worth it, because both the cost and benefit present themselves only when decimation is used, and I think the prominent artifacts are worse than it being a little more cpu intensive.

Other Notes

For background, including how to reproduce the issue described here, see #132

There also might be a better way to eliminate/reduce these artifacts; I'm not a sound engineer and I haven't done a lot of research on this topic.

(This change will also affect the FFT spectrum view, in addition to the 2D spectrogram view, but that seemed like it would be harder to demonstrate visually)

@@ -43,9 +43,6 @@ def __init__(self):
def analyzelive(self, samples):
samples = self.decimate(samples)

# uncomment the following to disable the decimation altogether
# decimation = 1
Copy link
Author

Choose a reason for hiding this comment

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

I tried setting self.decimation = 1 here, and that just resulted an error on the next line, "Operands cannot be broadcast together with shapes (4096,) (256,)"; so I think this comment is outdated

Copy link
Author

@egitto egitto Jan 23, 2020

Choose a reason for hiding this comment

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

(I also tried using self.decimation = 1 and "fixing" the below line to make the shapes line up, but that just resulted in this error instead; I guess the value of self.decimation affects a bunch of places)
image

fft = rfft(samples * repeat(self.window, len(samples)//len(self.window)))

Copy link
Author

Choose a reason for hiding this comment

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

oh, lol, actually settings self.decimation = 1 works just fine, you just have to do it in the set_maxfreq function so that it never sets it to any non-1 value
image

# the simplest way
samples = samples[:, 0]
samples = samples.mean(axis=1)
# the fastest way
Copy link
Author

Choose a reason for hiding this comment

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

(I didn't actually profile this, idk how much computation time goes here vs actually doing the fft and rendering)

Copy link
Author

Choose a reason for hiding this comment

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

hm, I just profiled (with a value of self.decimation == 16) and with the faster way, the decimation function takes about 7.05% of a the FFT's duration, and with the slower way it takes about 93.1% of the FFT's duration. So, a pretty significant cost, assuming the FFT is the expensivest part of this process.

@tlecomte
Copy link
Owner

Thank you @egitto for looking at this problem. Given your results, I suggest to remove the decimation completely. I've sent PR #137 for that. I'll merge if it builds fine.

As you found, using the mean still gives artifacts because it's not a good-enough low-pass filter for the decimation. Ideally we would use a better low-pass filter, like it is done in other parts of Friture, but here I don't think it's worth it.

@tlecomte
Copy link
Owner

I'm closing this since PR #137 has been merged. Thanks again @egitto for your investigation!

@tlecomte tlecomte closed this Nov 15, 2020
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

2 participants