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
Comments
Comment 1 by jteh on 2015-04-07 05:43 |
Comment 2 by leonarddr on 2015-04-07 10:29 |
Comment 3 by dkager (in reply to comment 2) on 2015-04-07 15:55
I think this code is...interesting. Someone should probably demystify this beepin' code at some point, i.e. do away with all those unsigned variables. |
Comment 4 by camlorn on 2015-04-07 15:57 |
Comment 5 by dkager on 2015-04-07 16:07 |
Comment 6 by nvdakor on 2015-04-07 16:18 |
Comment 7 by nvdakor on 2015-04-07 16:23 |
Attachment t5023.patch added by camlorn on 2015-04-07 17:05 |
Comment 8 by camlorn on 2015-04-07 17:11 |
Comment 9 by nvdakor on 2015-04-07 17:13 |
Comment 10 by dkager (in reply to comment 9) on 2015-04-07 17:22
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. |
Comment 11 by camlorn on 2015-04-07 17:40 |
Comment 12 by camlorn (in reply to comment 11) on 2015-04-07 17:42 |
Comment 13 by nvdakor on 2015-04-07 17:48 |
Comment 14 by dkager (in reply to comment 12) on 2015-04-07 17:57
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. :) |
Comment 15 by camlorn on 2015-04-07 18:08 |
Comment 16 by dkager (in reply to comment 15) on 2015-04-07 18:18
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:
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. |
Comment 17 by jteh on 2015-04-07 23:42 |
Comment 18 by nvdakor on 2015-04-08 00:08 |
Comment 20 by camlorn on 2015-04-08 00:24 |
Comment 21 by jteh on 2015-04-08 01:05 |
Comment 22 by dkager on 2015-04-08 09:49 |
Comment 23 by leonarddr on 2015-04-08 09:58 |
Comment 24 by dkager on 2015-04-08 13:49
I'm showing only one measurement for each but have reliably produced these same results in repeated tests. |
Comment 25 by leonarddr on 2015-04-08 14:09 Branch: Master
0.7703311105785815 Branch: t5023
0.3624246010443065`` |
Comment 26 by briang1 on 2015-04-08 14:38 Is the generation of beepseither in mouse tracking or progress bars a problem at the moment? |
Comment 27 by dkager on 2015-04-08 15:24
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. |
Comment 28 by camlorn on 2015-04-08 19:41 |
Comment 29 by dkager (in reply to comment 28) on 2015-04-08 20:10
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):
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:
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.)
Exactly.
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. |
Comment 30 by jteh on 2015-04-08 21:13 |
Comment 31 by camlorn on 2015-04-08 23:52 |
Comment 32 by dkager on 2015-04-09 06:10 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). |
Comment 33 by briang1 (in reply to comment 27) on 2015-04-09 09:19
|
Comment 34 by dkager on 2015-04-15 09:24
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. |
Comment 35 by jteh on 2015-04-15 09:36 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. |
Comment 36 by dkager on 2015-04-15 12:21 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. |
Comment 37 by camlorn on 2015-04-15 15:43 |
Comment 38 by jteh (in reply to comment 37) on 2015-04-15 22:43
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.
Maybe, maybe not; the code was written a long time ago and we knew a lot less back then.
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.
Laptop/tablet speakers suck. We all know this. Unfortunately, that's what many of us are dealing with when using a screen reader. |
Comment 39 by camlorn on 2015-04-16 04:45 |
Comment 40 by dkager on 2015-04-17 11:28 The following values are given as parameters:
We can then calculate some variables:
Introduce a temporary variable to hold the result of calling the C++ sin function, and simplify it a bit:
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:
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? 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:
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. |
Comment 41 by jteh on 2015-04-17 11:44 |
Comment 42 by dkager on 2015-04-17 11:52 |
Comment 43 by briang1 on 2015-04-19 07:33 |
Comment 44 by dkager (in reply to comment 43) on 2015-04-19 09:18
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. |
Comment 45 by briang1 on 2015-04-19 14:23 |
Comment 46 by James Teh <jamie@... on 2015-04-20 02:33
Changes:
|
Comment 48 by James Teh <jamie@... on 2015-05-01 12:20
Changes:
|
Comment 49 by jteh on 2015-05-01 12:20 |
…issues. Fixes #5023. Authors: Austin Hicks <camlorn38@gmail.com>, Davy Kager <mail@davykager.nl>
Reported by tspivey on 2015-04-07 05:39
STR:
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.
The text was updated successfully, but these errors were encountered: