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

tones.beep broken #5023

Closed
nvaccessAuto opened this issue Apr 7, 2015 · 48 comments
Closed

tones.beep broken #5023

nvaccessAuto opened this issue Apr 7, 2015 · 48 comments
Assignees
Labels
Milestone

Comments

@nvaccessAuto
Copy link

Reported by tspivey on 2015-04-07 05:39
STR:

  1. tones.beep(2205, 200)
  2. tones.beep(2206, 200)

These sound too far apart to be the correct frequencies. I tested with goldwave, and I can't tell the difference between these two; I can with NVDA.

@nvaccessAuto
Copy link
Author

Comment 1 by jteh on 2015-04-07 05:43
Confirmed, though I've no idea why it's happening. I'm guessing some sort of mathematical error. If anyone is interested in looking at this, the code is in nvdaHelper/local/beeps.cpp.

@nvaccessAuto
Copy link
Author

Comment 2 by leonarddr on 2015-04-07 10:29
I think it has to do with numbers being integers while they should be doubles.
const unsigned samplesPerCycle=static_cast<unsigned int>(sampleRate/hz);
This is 44100/pitch, and the result is e.g. the same for pitch 865/882. As you will notice, beep(865,x) till beep(882,x) is exact the same pitch, where 864 and 883 will sound lower and higher. We won't like 50.05 sample though, but rounding up the samples to early will certainly influence the pitch of the outcome.

@nvaccessAuto
Copy link
Author

Comment 3 by dkager (in reply to comment 2) on 2015-04-07 15:55
Replying to leonarddr:

I think it has to do with numbers being integers while they should be doubles.

I think this code is...interesting.
Anyway, here is a fix that rendered tones more accurately in the very brief tests I ran.

Someone should probably demystify this beepin' code at some point, i.e. do away with all those unsigned variables.

@nvaccessAuto
Copy link
Author

Comment 4 by camlorn on 2015-04-07 15:57
The whole function should be using floats and an entirely different formula and I am not sure who thought the current version was a good idea in the first place though it's possible I' missing context.
I will provide a patch shortly that should resolve the issues.

@nvaccessAuto
Copy link
Author

Comment 5 by dkager on 2015-04-07 16:07
I too would be interested to know who wrote this code (was it pulled from an external project?), why it isn't done in Python and what some of the logic is trying to accomplish. For instance, in the five minutes I spent looking at this function I couldn't figure out how exactly the total number of samples is computed.

@nvaccessAuto
Copy link
Author

Comment 6 by nvdakor on 2015-04-07 16:18
Hi,
I'd vote for using doubles for better precision (float is 32-bit, while double is 64-bit). We may run into issues regarding negative frequencies (we don't care about those, as they cannot exist). In case of negative values, we can tweak tones module to raise value error if negative numbers (real numbers, that is) are passed through the function (same can be said about duration).
I, too, wonder who wrote that code (I hope it wasn't Mick or Jamie). BTW, you might want to update the copyright year to 2015 as well.
Thanks for the solution.

@nvaccessAuto
Copy link
Author

Comment 7 by nvdakor on 2015-04-07 16:23
Hi,
Disregard my comment about using doubles - the code already uses doubles. The code looks fine and it resolves the problem for now. Thanks.

@nvaccessAuto
Copy link
Author

Attachment t5023.patch added by camlorn on 2015-04-07 17:05
Description:
Patch to fix generateBeep

@nvaccessAuto
Copy link
Author

Comment 8 by camlorn on 2015-04-07 17:11
Sorry, I haven't figured out how to do a comment and attach a file at the same time. I suspect I'm missing something incredibly obvious.
This patch outright replaces the old formula which is completely nonsensical unless you're the one who wrote it with a new one, removes some aliasing issues, and sounds audibly one-for-one with Audacity.
Technically, if the buffer is long enough, this function will do odd things. Long enough is upwards of 5 minutes at a minimum, and I suspect it's upwards of an hour in practice. But the only way to test is to listen, and I am not going to listen to an hour of sine wave.
Note that this will affect mouse tracking audio. I don't mind the new one, but if others want it to be the same, someone will need to tweak numbers.

@nvaccessAuto
Copy link
Author

Comment 9 by nvdakor on 2015-04-07 17:13
Hi,
Davy, would you like to include the patch yourself or would you like me to combine your work with that of Austin's patch? It might be best to combine it at your end so lead developers can work from your branch alone.
Thanks.

@nvaccessAuto
Copy link
Author

Comment 10 by dkager (in reply to comment 9) on 2015-04-07 17:22
Replying to nvdakor:

Hi,

Davy, would you like to include the patch yourself or would you like me to combine your work with that of Austin's patch? It might be best to combine it at your end so lead developers can work from your branch alone.

Thanks, I will combine Austin's patch since it's actually much nicer. However I'd vote for sticking to static_cast (or to remove all C++-style casts) for consistency. Also I believe one could optimize the for-loop a little bit. I'll add new commit shortly.

@nvaccessAuto
Copy link
Author

Comment 11 by camlorn on 2015-04-07 17:40
Yeah, cast consistency is not something I thought about. For what it's worth, that's also the first time I've actually seriously seen someone using static_cast in a mathematical context, but semantically it's the same so pick whichever if you think it's important.

@nvaccessAuto
Copy link
Author

Comment 12 by camlorn (in reply to comment 11) on 2015-04-07 17:42
Also, don't optimize the for loop. I promise it's fast enough. I promise it's fast enough even if we scrap the C function and reimplement it in Python. Code clarity is part of why we got here in the first place, so choose that over anything.

@nvaccessAuto
Copy link
Author

Comment 13 by nvdakor on 2015-04-07 17:48
Hi,
I agree with the last comment - let's get the right solution now, then once it works, work on optimizing it. Besides, in official releases, the compiler will optimize the code for us, so no need to worry about optimization for now.
Thanks.

@nvaccessAuto
Copy link
Author

Comment 14 by dkager (in reply to comment 12) on 2015-04-07 17:57
Replying to camlorn:

Also, don't optimize the for loop. I promise it's fast enough. I promise it's fast enough even if we scrap the C function and reimplement it in Python. Code clarity is part of why we got here in the first place, so choose that over anything.

My bad. In this context, "optimize" referred to readability, not to speed (hence why I was wondering why this is being done in C++ in the first place). I'd rather have a few extra parens than needing to remember my arithmetic operator precedence rules. :)

@nvaccessAuto
Copy link
Author

Comment 15 by camlorn on 2015-04-07 18:08
Okay. That's better. But I'm not sure what precedence issues I have here, and I'd say--as someone who can work on this kind of code effectively--the people who can work on this kind of code effectively know them. By heart. Add it if you want, I don't care at this point. But for most of us who actually understand the formula I used, precedence is like hello world. And I'm not sure how you "fix" this further without understanding that far.
I'll watch my e-mail and stuff and pop in if there's something that actually needs me, but this should be done as far as I can tell.

@nvaccessAuto
Copy link
Author

Comment 16 by dkager (in reply to comment 15) on 2015-04-07 18:18
Replying to camlorn:

Okay. That's better. But I'm not sure what precedence issues I have here, and I'd say--as someone who can work on this kind of code effectively--the people who can work on this kind of code effectively know them. By heart. Add it if you want, I don't care at this point. But for most of us who actually understand the formula I used, precedence is like hello world. And I'm not sure how you "fix" this further without understanding that far.

I know my rules and hello worlds, but assuming others did too has bitten me before. Anyway, I left your patch as it was except for:

  • Updated copyright years.
  • Made it a bit more consistent with what was already there.
  • Tiny formatting fixes to the header file.

Here's the branch again (I amended the original commit): https://bitbucket.org/dkager/nvda/branch/t5023

I can confirm it sounds much better now, which IMHO is an improvement in all respects.

@nvaccessAuto
Copy link
Author

Comment 17 by jteh on 2015-04-07 23:42
The original code was in Python, but it was causing noticeable performance problems on netbooks with Atom processors (#396). Thus, it was reimplemented in C++ (9ae75fd). While I agree the current code is buggy and ugly, I can't accept a complete rewrite unless someone can test/guarantee that this won't regress performance on Atom processors.

@nvaccessAuto
Copy link
Author

Comment 18 by nvdakor on 2015-04-08 00:08
Hi,
I'm sure more recent Atom processors shouldn't exhibit performance regressions, as they are based on a more modern microarchitecture. This may applie to really old Atom processors such as the one mentioned in #396 (I believe it is Atom 230).
Should we have a try build so that those with Atom processors can try the modifications?
Thanks.

@nvaccessAuto
Copy link
Author

Comment 20 by camlorn on 2015-04-08 00:24
Even on an Atom processor, the code I gave should be fine. I'd be willing to put money on this. The rewrite is in C++.
If it's really too slow, switch all occurrences of double with float and it probably won't be anymore.
If my rewrite is slow on Atom, I want to know why. The only thing I can think of for the Python version having had an issue is cache misses, but that shouldn't be any more of an issue for the code I gave than it was for the old function that it replaced.

@nvaccessAuto
Copy link
Author

Comment 21 by jteh on 2015-04-08 01:05
As I noted in #396, the Python issue was very likely due to the use of immutable strings.

@nvaccessAuto
Copy link
Author

Comment 22 by dkager on 2015-04-08 09:49
Would it be enough to, pardon the pun, timeit as a poor man's regression test? Should be adequate if you generate a long (60,000 ms) and low-frequency (50 Hz) tone.

@nvaccessAuto
Copy link
Author

Comment 23 by leonarddr on 2015-04-08 09:58
In any case, I'd like to vote for merging this branch into next for testing purposes rather than creating a separate build for it.

@nvaccessAuto
Copy link
Author

Comment 24 by dkager on 2015-04-08 13:49
First of all, I don't think @jteh has anything to worry about, and props to Austin. Secondly, I'm going to do another commit later today that changes the C++ code to stop using unsigned chars and ints. See my results below:

import timeit

# Branch: master
>>> timeit.timeit('tones.beep(50, 60000)', setup='import tones', number=1)

0.46212753180210875

# Branch: t5023
>>> timeit.timeit('tones.beep(50, 60000)', setup='import tones', number=1)

0.24845931312447078

# Branch: t5023, stop pointlessly using unsigned data types
>>> timeit.timeit('tones.beep(50, 60000)', setup='import tones', number=1)

0.17771994895494458

I'm showing only one measurement for each but have reliably produced these same results in repeated tests.

@nvaccessAuto
Copy link
Author

Comment 25 by leonarddr on 2015-04-08 14:09
Looks impressive, though still should be tested on older platforms imo.
Here are the results on an I7-4712MQ:
``import timeit

Branch: Master

timeit.timeit('tones.beep(50, 60000)', setup='import tones', number=1)

0.7703311105785815

Branch: t5023

timeit.timeit('tones.beep(50, 60000)', setup='import tones', number=1)

0.3624246010443065``

@nvaccessAuto
Copy link
Author

Comment 26 by briang1 on 2015-04-08 14:38
Hi, just spotted this ticket. For those of us not following this, what exactly is the problem now and does this fix have any problems with the older AMD processors which do not have the sse2 instruction sets.

Is the generation of beepseither in mouse tracking or progress bars a problem at the moment?
Just wondered

@nvaccessAuto
Copy link
Author

Comment 27 by dkager on 2015-04-08 15:24
The code now uses signed integers for everything but the output buffer. As shown in comment:24 this gives a ~30% speed-up. I updated the branch with another commit. Remaining questions:

  1. How does this run on slower machines?
  2. Is the pitch of the tones still accurate?

FYI: Switching to floats instead of doubles yielded no speed-up on my Intel T9550, so I would suggest staying with doubles.

@briang1: The code doesn't explicitly rely on SSE2 or any other special instructions, their use depends on whatever the scons scripts set and will not change with these patch. The ticket's initial description aptly explains the initial problem.

@nvaccessAuto
Copy link
Author

Comment 28 by camlorn on 2015-04-08 19:41
The only way for us to tel for 100% certainty that this is generating the right frequency is to perform some sort of test against a reference implementation, i.e. audacity. This can be done but is nontrivial.
This code is nearly identical to the Libaudioverse sine node, which I tested by ear against the Pyo reference implementation in the REPL. I will make a point of testing it by ear against one of my reference implementations this evening.
The performance concerns here are almost certainly not an issue. It does not explicitly require SSE and it is as cache friendly as the old one. The only potential optimization that is left is this: switch to floats for the panning values.
Beyond that we are talking about SSE. I can provide an SSE implementation, it's actually not that hard, but SSE is mostly pointless in this case because we need to write out as 16-bit shorts. I am not aware of any modern CPU (since about 2005) that is lacking SSE2 and, unless specified otherwise in scons, this is the default in VS2012. If there is genuine and significant interest in this for a good reason, I will skim the SSE intrinsic list to see if the conversion from floats to shorts can actually be done cheaply.
But honestly, fast enough is good enough. If you want to go optimize something, go optimize something that will actually make a noticeable difference. This is a micro-optimization of code that was faster than the original version when someone bothered to time it, I'm not sure why people are so focused on squeezing every last CPU cycle out of it.

@nvaccessAuto
Copy link
Author

Comment 29 by dkager (in reply to comment 28) on 2015-04-08 20:10
Replying to camlorn:

The only way for us to tel for 100% certainty that this is generating the right frequency is to perform some sort of test against a reference implementation, i.e. audacity.

If the formula is as standardized as you are implying one could also formally proof the correctness of this function, but IMHO an audible test is enough.

Speaking of which, I'd like to ask about beeps.cpp(25):

    totalSamples+=samplesPerCycle-(totalSamples%samplesPerCycle);

I believe this is trying to add enough samples so that the last cycle of the sine wave will end at zero. I'm not sure if the logic in this line is correct. Shouldn't this be changed to:

    totalSamples+=totalSamples%samplesPerCycle;

Neither of the above sound good audibly and have the potential of deviating quite a bit from the requested length depending on the wave length. (So yes, I do mean to seriously discuss this before dismissing the ticket as fixed.)

The performance concerns here are almost certainly not an issue. It does not explicitly require SSE and it is as cache friendly as the old one.

Exactly.

This is a micro-optimization of code that was faster than the original version when someone bothered to time it, I'm not sure why people are so focused on squeezing every last CPU cycle out of it.

Not sure if I'm representative of the people, but I changed the code because it was ugly. Now it's less ugly '''and''' a little faster. Can't lose there.

@nvaccessAuto
Copy link
Author

Comment 30 by jteh on 2015-04-08 21:13
Steady on, people. I didn't ask for this code to be hand written in machine code for light speed performance. I asked for a reasonable guarantee that it wouldn't regress on the Atom processors that originally had the problem in #396. Timing isn't necessarily indicative, as lower end processors might not perform as well with certain operations; it's not just a matter of directly proportionate speed. Regardless, I'm reasonably satisfied that this code shouldn't regress and we'll call for testing once it's in next. First, either Mick or I (probably Mick) needs to review this.

@nvaccessAuto
Copy link
Author

Comment 31 by camlorn on 2015-04-08 23:52
The maximum deviation from the length is inversely proportional to the frequency. At frequencies we care about, the deviation will be less than 10 MS. Specifically, the maximum deviation at 100 hz is 1/100th of a second, and it drops rapidly from there.
I think the current one is indeed trying to align the boundaries of the buffer on periods. The following expression is what I usually use for this sort of thing: totalSamples = (totalSamples/samplesPerCycle+1)*samplesPerCycle. This is intentionally integer division and will fail hard if you use floats or doubles. You should also be able to do totalSamples += samplesPerCycle-totalSamples%samplesPerCycle. I personally prefer the former. If you can't get it working, I'll recreate my Bitbucket account and look; if worst comes to worst we can envelope it.
As for the formula, no, you can't prove the function. Proving the formula doesn't imply that the function doesn't have a stupid bug in it or something. But the short version is that sine is periodic on 2pi. Consequently, we multiply x by 2pi to get it periodic on 1, multiply by 1/f to get it repeating f times a second, and then plug in time in samples over samplerate to get time in seconds from time in samples. More formally, we sample the continuous sine wave with frequency f.
testing this is difficult because it's in NVDA and we have no easy framework to test against. If I extracted it and got it into a Python with Numpy, I could give a test case. I did just test it against Libaudioverse however, and they are audibly identical. libaudioverse is an implementation I trust, though I'm biased given that I wrote all of Libaudioverse.
And also, for the record, floats versus doubles. It occurred to me later that, if you change everything to floats, it's not going to matter unless you replace sin with sinf because that's the majority of the computation. I make no guarantee that it will affect anything, but if something does somehow actually come up along the lines of old processors it's something to try at least.

@nvaccessAuto
Copy link
Author

Comment 32 by dkager on 2015-04-09 06:10
Maybe this should've been discussed on a channel that doesn't e-mail everybody about updates (sorry people!).
As for the proof: it's this kind of proving that reassured me I didn't want to be a mathematician. :)
Regression testing: the most modest CPU I can test on is an Atom N450, which is still relatively modern and is likely to have an FPU powerful enough to deal with this function.

Edit: Ugh, ignore my previous comment, the calculation of totalSamples is fine. The only possible improvement would be to add one sample so that you end on the first sample of a new cycle (which should be zero).

@nvaccessAuto
Copy link
Author

Comment 33 by briang1 (in reply to comment 27) on 2015-04-09 09:19
Well, you may well think that, but I'd not have asked if I had got it for sure. However, one thing I've noticed a lot about bleeps from nvda is that they seem not to switch on and off at zero crossing points. Instead you get clicks and plop sounds. This is particuarlly obvious on slower machines it seems, particularly those with single core processors like AMD Celeraons, ie the ones refered to in my last comment, and Pentium 4s, such as older laptops and some desktops.
These machines are still in use around the planet I'm sure as they are here in the Uk, for non exacting work.
Replying to dkager:

The code now uses signed integers for everything but the output buffer. As shown in comment:24 this gives a ~30% speed-up. I updated the branch with another commit. Remaining questions:

  1. How does this run on slower machines?
  2. Is the pitch of the tones still accurate?

FYI: Switching to floats instead of doubles yielded no speed-up on my Intel T9550, so I would suggest staying with doubles.

@briang1: The code doesn't explicitly rely on SSE2 or any other special instructions, their use depends on whatever the scons scripts set and will not change with these patch. The ticket's initial description aptly explains the initial problem.

@nvaccessAuto
Copy link
Author

Comment 34 by dkager on 2015-04-15 09:24
Okay, I'm voting to not use this new implementation of generateBeep after all. I tested the code on my netbook (Intel Atom N450) and found the following (point 3 is a showstopper IMHO).

  1. The time between pressing Enter and the beep playing isn't perceivably different between the master and t5023 branches. Tested in the Python console with tones.beep(440, 200).
  2. The tone does crackle and "stutter" quite a bit when the CPU is under heavy load, for both master and t5023. But then, Espeak itself suffers from the same problem on these low-end systems. Not an issue in the beep generating code.
  3. On the small speakers in the netbook the (perfect) sine wave from t5023 is nearly inaudible downward from 500 Hz. The version in master doesn't have this problem. I think we need to square off the sine somehow or use a different wave.

As for performance: if it's still not fast enough right now it probably makes more sense to generate one cycle of a wave, then memcopy it into the buffer as many times as possible. No tinkering with SSE. However, the real problem is that we don't want a sine wave.

@nvaccessAuto
Copy link
Author

Comment 35 by jteh on 2015-04-15 09:36
As I said to camlorn separately, regardless of whether it's ugly or not, the current implementation is very intentionally not a sine wave. What I don't quite know is what wave it was intended to be; Mick may know more about this. The problem is that a pure square wave is probably too harsh. Mick noted that maybe a triangle wave would work.

At the end of the day, we really don't want to change the sound of this except for the pitch issues. The question is how to achieve that while still fixing the bug this ticket was originally opened to fix. If the code gets prettier in the process, that's great.

@nvaccessAuto
Copy link
Author

Comment 36 by dkager on 2015-04-15 12:21
I added another commit to the branch which reverts back to the original waveform while keeping the other refactorings. So in short, all this really does is use doubles for internal calculations, then cast the final samples to shorts. On my laptop speakers the tone sounds the same as the one in master and it pitches up and down correctly.

The actual wave generation looks (and sounds) like a square wave with sloping edges, but the algorithm is a little confusing. I don't feel like trying to demystify it since the new code sounds the same yet is cleaner, faster (yes I know...) and more accurate.

EDIT: amended because I made the exact same mistake that was in the original code. Also added a terse comment warning against this.

@nvaccessAuto
Copy link
Author

Comment 37 by camlorn on 2015-04-15 15:43
First, unless someone gives a really good reason that I'm missing, the original formula under discussion here is broken. I have absolutely no idea what that's periodic on, but I can't see any way in which it is hertz. Perhaps someone did something really clever with math and didn't comment it. We should probably move to semitones anyway, because hertz don't actually do what people expect given that human hearing is logarithmic in regards to pitch.
Second, did you compile in release mode? Because if you're doing all the tests in debug and without optimization, the performance will be horrible. I'm assuming that you did.
Third, we can use a wavetable but someone needs to say what the wave should be. This would just be a linear interpolation across a table of points. I can make it from literally any mono wave file containing exactly one period of the waveform or, for simple waves, just write it out by hand.
And fourth, I'd love to know why we're having audibility issues at 500 hz and below. 500 hz is well in the middle of the "perceptible" range of pitches if you view it as notes on a keyboard, somewhere right around middle C. This seems like a very odd thing for a manufacturer to have a problem with. What might work to fix this nicely is adding some harmonics. If we want to try this, I can provide a patch sometime around next Tuesday, but the basic idea is to sum some sine waves at whole-number multiples of the frequency. In reality, we'd probably want to use the same harmonics that are in a C-chord, namely 2 steps up and 1 and a half steps up. I can get the formulas for this trivially, though it is worth noting that they are technically decimals.

@nvaccessAuto
Copy link
Author

Comment 38 by jteh (in reply to comment 37) on 2015-04-15 22:43
Replying to camlorn:

First, unless someone gives a really good reason that I'm missing, the original formula under discussion here is broken. I have absolutely no idea what that's periodic on, but I can't see any way in which it is hertz.

I can't comment on the formula--Mick might know more--but what I can tell you is that at least most of the time (barring the rounding issues), the pitch is correct to my ears; my pitch is accurate to within a few hz most of the time.

Perhaps someone did something really clever with math and didn't comment it.

Maybe, maybe not; the code was written a long time ago and we knew a lot less back then.

We should probably move to semitones anyway, because hertz don't actually do what people expect given that human hearing is logarithmic in regards to pitch.

That would be ugly for the two primary use cases. Progress bar beeps have 25 steps per octave, not 12. Mouse audio coordinates are continuous, not discrete.

And fourth, I'd love to know why we're having audibility issues at 500 hz and below.

Laptop/tablet speakers suck. We all know this. Unfortunately, that's what many of us are dealing with when using a screen reader.

@nvaccessAuto
Copy link
Author

Comment 39 by camlorn on 2015-04-16 04:45
When I have some time, I will go and find the original Python version if I can. We hopefully still have a branch for the other ticket that I can track back to its beginning. That really should put this ticket to rest once and for all.

@nvaccessAuto
Copy link
Author

Comment 40 by dkager on 2015-04-17 11:28
(This comment falls into the category of "Please correct me if I'm wrong...")
Here is part of the proof that the old code is indeed correct. This is incomplete because I show that the code works for one frequency, then happily assume that it will thus work for all frequencies (omitting the inductive step). Also, I didn't include stereo panning and amplitude calculations since their correctness was never questioned in the first place.

The following values are given as parameters:

length = 1000
hz = 441

We can then calculate some variables:

samplesPerCycle = static_cast<int>(sampleRate/hz)
= static_cast<int>(44100/441)
= 100
totalSamples = static_cast<int>((length/1000)/(1/sampleRate))
= static_cast<int>((1000/1000)/(1/44100))
= 441
sinFreq = (2*PI)/(sampleRate/hz)
= (2*PI)/(44100/441)
= (2*PI)/100

Introduce a temporary variable to hold the result of calling the C++ sin function, and simplify it a bit:

tmp = sin((sampleNum%sampleRate)*sinFreq)*2
{ eliminate sampleNum%sampleRate, see below }
= sin(sampleNum*sinFreq)*2
{ eliminate outer multiplication, see below }
= sin(sampleNum*sinFreq)
{ definition of sinFreq }
= sin(sampleNum*((2*PI)/100))

The modulo calculation in the definition of tmp above can be eliminated because all it does, apparently, is guarding against overflows when generating very long tones. For the multiplication by 2, see below.

Note that 2*PI is precisely the period of the sine function. Thus the following fragment will render only zeroes:

sampleNum*(2*PI)

That's the constant function y = 0. The question then is: how much do you need to compress the interval 2_PI in order for one period to last 100 samples, with 100 being the previously derived number of samples in each cycle?
Well, if 2_PI leads to 1 sample per cycle, then (2*PI)/100 leads to 100 samples per cycle. And that is the very definition of sinFreq! It also shows that using an integer number in this division, as the original code did, will produce inaccurate results.

Having shown that the variable tmp as defined above is correct, it immediately follows that tmp*2 is correct, because all it does is expand the wave to reach points twice as high and twice as low as the original wave, causing steeper slopes.

With the knowledge that the sine function fluctuates between -1 and 1, we also know that tmp*2 fluctuates between -2 and 2. So finally what remains is:

sample = min(max(tmp*2,-1),1)

Keep in mind that max(-2,-1) = -1 and min(2,1) = 1.

In conclusion, sample values move freely in the interval [-1..1] and are capped outside of this range. The result is a square wave with sloping edges. If this is the waveform the original developer was aiming for, then the code may be inefficient but it is not broken.

My vote goes toward marking this ticket as resolved and opening a separate one for discussion of different waveforms and other hacks.

@nvaccessAuto
Copy link
Author

Comment 41 by jteh on 2015-04-17 11:44
FYI, the original Python code was written by Mick. It was then ported to C++ by Aleksey.

@nvaccessAuto
Copy link
Author

Comment 42 by dkager on 2015-04-17 11:52
Barring mathematical mistakes in comment:40, I think it safe to say Mick did a good job. The one problem is that at 22050 Hz and 44100 Hz, the function will output only zeroes (or something close to zero because of rounding errors). This is due to properties of the sine function.
Maybe we should add some checks to tones.beep to make sure the requested frequency is below 22050 Hz and that the left/right panning values are between 0 and 100?

@nvaccessAuto
Copy link
Author

Comment 43 by briang1 on 2015-04-19 07:33
Why would you worry, unless you had dogs or bats using the computer?
Surely the frequency should not go above the human hearing limits.
I was led to believe that the sample rate used meant you could not produce sounds over half the rate with accuracy, and low distortion and artifacts.

@nvaccessAuto
Copy link
Author

Comment 44 by dkager (in reply to comment 43) on 2015-04-19 09:18
Replying to briang1:

I was led to believe that the sample rate used meant you could not produce sounds over half the rate with accuracy, and low distortion and artifacts.

Correct (as per Nyquist). Which is why capping the frequency at something under 22050 Hz seems a good idea (also because requesting really high frequencies from this code can throw exceptions). The same is true for the left/right panning parameters, which can programmatically be set to negative numbers or numbers greater than 100. The code should probably round those off to acceptable values or simply return an error.

@nvaccessAuto
Copy link
Author

Comment 45 by briang1 on 2015-04-19 14:23
When I was young my hearing cut off around 12000hz, and now its about 9000hz, and I'd seriously doubt anyone would be putting out tones at these frequencies really, as a lot of people have poor hearing and poor headphones or speakers anyway!

@nvaccessAuto
Copy link
Author

Comment 46 by James Teh <jamie@... on 2015-04-20 02:33
In [a05278b]:

Merge branch 't5023' into next

Incubates #5023.

Changes:
Added labels: incubating

@nvaccessAuto
Copy link
Author

Comment 48 by James Teh <jamie@... on 2015-05-01 12:20
In [7b4a196]:

Refactor beep generation code to fix incorrect frequencies and other issues.

Fixes #5023.
Authors: Austin Hicks camlorn38@gmail.com, Davy Kager mail@davykager.nl

Changes:
Removed labels: incubating
State: closed

@nvaccessAuto
Copy link
Author

Comment 49 by jteh on 2015-05-01 12:20
Changes:
Milestone changed from None to 2015.2

@nvaccessAuto nvaccessAuto added this to the 2015.2 milestone Nov 10, 2015
jcsteh added a commit that referenced this issue Nov 23, 2015
…issues.

Fixes #5023.
Authors: Austin Hicks <camlorn38@gmail.com>, Davy Kager <mail@davykager.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants