-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Refactor the engine to process samples in the range of [-1.0, 1.0]. Fixes Bug #1204039. #126
Conversation
@ywwg could you please test that vinyl control still works? :) |
Oops, I forgot shoutcast/recording. |
I'll take a look |
Thanks. The tests pass and the actual code changes are pretty non-controversial -- I just don't have a VC setup to test. |
Debug [AnalyserQueue 1]: Beat calculation will not start Program received signal SIGFPE, Arithmetic exception. Thread 28 (Thread 0x7fff81bf5700 (LWP 1180)): Thread 27 (Thread 0x7fff823f6700 (LWP 1179)): Thread 26 (Thread 0x7fff8b9a3700 (LWP 1140)): Thread 25 (Thread 0x7fff99212700 (LWP 1139)): ---Type to continue, or q to quit--- Thread 23 (Thread 0x7fff99a13700 (LWP 1137)): Thread 22 (Thread 0x7fff9a214700 (LWP 1136)): Thread 21 (Thread 0x7fff9b4dc700 (LWP 1135)): Thread 20 (Thread 0x7fffacd75700 (LWP 1134)): Thread 19 (Thread 0x7fffae603700 (LWP 1133)): Thread 18 (Thread 0x7fffafafe700 (LWP 1132)): Thread 17 (Thread 0x7fffbd725700 (LWP 1131)): Thread 16 (Thread 0x7fffbefb3700 (LWP 1130)): Thread 15 (Thread 0x7fffc4856700 (LWP 1129)): ---Type to continue, or q to quit--- Thread 13 (Thread 0x7fffc7df2700 (LWP 1127)): Thread 12 (Thread 0x7fffc6d05700 (LWP 1124)): Thread 7 (Thread 0x7fffdc891700 (LWP 1119)): Thread 6 (Thread 0x7fffdd425700 (LWP 1118)): Thread 3 (Thread 0x7fffdec28700 (LWP 1114)): Thread 2 (Thread 0x7fffe015f700 (LWP 1113)): Thread 1 (Thread 0x7ffff7f967c0 (LWP 1099)): |
I will be going on vacation tomorrow so I may be slow in retesting |
It is possible to test vinyl without a setup, although it's tricky:
That will cause the samples from the serato CD to feed into the vinyl control input of deck 2, so deck 2 should start playing. |
Hm. That segfault smells like we're writing past VinylControlXwax's |
The crash happens right when the play button illuminates so I suspect the pitch smoothing code. |
Right -- |
…ixes Bug #1204039. * Move Vinyl Control pre-amp to VinylControlXwax (no longer affects microphone/passthrough). * Switch PortAudio input sample format to paFloat32. * Remove SHRT_MAX conversions on input and output samples. * Update VUMeters, clipping, and dithering code to deal with the new ranges. * Clean up EngineMicrophone/EnginePassthrough/EngineDeck handling of inputs. * Update tests to deal with the new sample ranges. This does not change SoundSource. SoundSources still read 16-bit samples and we simply divide by SHRT_MAX in AnalyserQueue and CachingReaderWorker. A future performance improvement can remove this and use the native decoder API to request normalized samples directly.
Conflicts: src/vinylcontrol/vinylcontrol.cpp src/vinylcontrol/vinylcontrol.h src/vinylcontrol/vinylcontrolxwax.cpp src/vinylcontrol/vinylcontrolxwax.h
|
||
bool bHaveSignal = fabs((float)samples[0]) + fabs((float)samples[1]) > MIN_SIGNAL; | ||
bool bHaveSignal = fabs(pSamples[0]) + fabs(pSamples[1]) > MIN_SIGNAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: Need to update MIN_SIGNAL to account for normalization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -102,9 +102,12 @@ | |||
m_iCrossFadeSamples(0), | |||
m_iLastBufferSize(0) { | |||
|
|||
// Generate dither values | |||
// Generate dither values. Before engine samples were normalized between | |||
// [-1.0, 1.0], dithering values were in the range [-0.5, 0.5]. Now that we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird phrasing in this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It reads fine to me but maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the commas are making it confusing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it sounds like we used to normalize between -1 and 1, but now we normalize from -1 to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yea that's meant to read "Before we did X, we did Y".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-worded to something hopefully more clear.
I run my sound card at 24 bit resolution... is that something a) that we/pulseaudio even support, or b) would be affected by this change, for better or worse? |
I believe this change should be good for 24-bit mode. Previously we took input as 16-bit PCM so PA would have to adapt from 24-bit to 16-bit PCM and we would convert from 16-bit PCM to float. Now, PA converts from 24-bit PCM to [-1.0, 1.0] normalized floats when it delivers input to us and takes out [-1.0, 1.0] floats and converts them back to 24-bit PCM on output. Basically, we save a 16-bit -> 24-bit conversion. In general (regardless of bit-depth your soundcard is using), this saves on a bunch of multiplying input and output by SHRT_MAX. |
Any sign of that segfault you saw? |
no sign of the segfault. |
LGTM |
Refactor the engine to process samples in the range of [-1.0, 1.0]. Fixes Bug #1204039.
microphone/passthrough).
This does not change SoundSource. SoundSources still read 16-bit samples and we
simply divide by SHRT_MAX in AnalyserQueue and CachingReaderWorker. A future
performance improvement can remove this and use the native decoder API to
request normalized samples directly.