-
Notifications
You must be signed in to change notification settings - Fork 73
Audio Settings Dialog: take II #21
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
base: master
Are you sure you want to change the base?
Conversation
updating from 0.5.2
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
…ase and compute_waveform These round() methods were profiling at 5%! Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
http://www.fftw.org/doc/SIMD-alignment-and-fftw_005fmalloc.html#SIMD-alignment-and-fftw_005fmalloc Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
|
in algo.c the numeric shift needs to be reduced to 15, or there is an integer rollover in calibration mode. Sorry about that. |
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
I just so tired of having to edit the names each time. Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
Signed-off-by: jakelewis3d <jakelewis3d@gmail.com>
|
My bad, I'd left a few #defs in that I need to build in eclipse IDE. Now removed. |
Makefile.am
Outdated
| src/audio_settings.c \ | ||
| src/audio.h \ | ||
| src/gtk/gtkHelper.c \ | ||
| src/gtk/gtkHelper.h \ |
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.
This is not valid syntax and is rejected by automake. Line 20 should not have whitespace and there should be no backslash continuation on the final line.
src/gtk/gtkHelper.c
Outdated
| */ | ||
|
|
||
| #include <gtk/gtk.h> | ||
| #include "gtk/gtkHelper.h" |
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.
Don't think this is the correct path for your header. This is relative to the location of the current file. So this means "src/gtk/gtk/gtkHelper.h", which is not the correct location. It should be "gtkHelper.h".
But I don't know if you really need a new directory here. The interface code, still in src, is just as much Gtk code as this is.
| * Created on: Oct 24, 2019 | ||
| * Author: jlewis | ||
| * This program is free software; you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License version 2 as |
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.
Comment formatting is mangled here.
src/gtk/gtkHelper.c
Outdated
| cairo_pattern_get_rgba (cairocolor, | ||
| &red,&green,&blue,&alpha); | ||
| char rgbaStr[1024]; | ||
| sprintf(rgbaStr, "rgba(%f,%f,%f,%f)", 255*red, 255*green, 255*blue, alpha); |
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.
Should at least use snprintf(). But glib has functions for safe strings now that would be better.
src/gtk/gtkHelper.c
Outdated
| // spin button | ||
|
|
||
| GtkWidget * createSpinButtonContainer(const gchar*name, | ||
| double min, double max, |
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.
Doesn't seem like your indent here is correct. Maybe you are viewing this file with non-standard top stops?
src/interface.c
Outdated
| } | ||
|
|
||
|
|
||
| void console(char *format,...)//this is better for eclipse development |
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.
This doesn't seem related to audio settings dialog. Perhaps a different commit? I don't actually see it used.
src/interface.c
Outdated
| w->snapshot_POS_button[4] = gtk_button_new_with_label("9U"); | ||
| w->snapshot_POS_button[5] = gtk_button_new_with_label("12U"); | ||
| for (int i = 0; i < POSITIONS; i++) { | ||
| GtkWidget *button = w->snapshot_POS_button[i]; |
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.
How about:
static const char* const labels[] = {"DD", "DU", "3U", ... };
GtkWidget *button = w->snapshot_POS_button[i] = gtk_button_new_with_label(labels[i]);
src/output_panel.c
Outdated
| if( strcmp(gtk_button_get_label(b) , MAG_INC)==0) | ||
| paperstrip_zoom_var *= MAG_SCALE; | ||
| else | ||
| paperstrip_zoom_var /= MAG_SCALE; |
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.
whitespace errors
| } | ||
|
|
||
| gboolean interface_setFilter(struct main_window *w, gboolean bLpf, double freq){ | ||
| if(w->computer){ |
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.
Why would w->computer not be set?
| return 1; | ||
|
|
||
| PaStreamParameters inputParameters; | ||
| inputParameters.channelCount = 2; // Stereo |
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.
What's the point of setting these here? And why stereo? The audio is converted to mono so wouldn't mono be better to capture?
| inputParameters.channelCount = info.channels = channels; | ||
|
|
||
| if(inputParameters.device == Pa_GetDefaultInputDevice()){ | ||
| debug("Pa_OpenDefaultStream dev:%d channels:%d\n",inputParameters.device, inputParameters.channelCount); |
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.
What's the reason to not just use Pa_OpenStream() once, for any device? Is there some difference in calling Pa_OpenDefaultStream().
src/audio_settings.c
Outdated
|
|
||
| #include "audio.h" | ||
|
|
||
| #include <gtk/gtkHelper.h> |
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.
The path for this header is wrong. This isn't a system header.
|
Opening the audio setting dialog always causes tg to crash. |
| CONFIG_FIELDS(LOAD); | ||
|
|
||
| w->audioInputStr = g_key_file_get_string(w->config_file, "tg", "audioInputStr", NULL); | ||
| w->audioInterfaceStr= g_key_file_get_string(w->config_file, "tg", "audioInterfaceStr", NULL); |
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.
If the config file doesn't have this key, then w->audioInterfaceStr is NULL and the audio dialog will crash.
| static const char * filterDev( PaDeviceIndex devIndex, int nominal_sample_rate){ | ||
| const PaDeviceInfo *info = Pa_GetDeviceInfo(devIndex); | ||
| if (info->maxInputChannels > 0){ | ||
| PaStreamParameters inputParameters; | ||
| inputParameters.channelCount = 2; // Stereo | ||
| inputParameters.sampleFormat = paFloat32; | ||
| // inputParameters.suggestedLatency = ; | ||
| inputParameters.hostApiSpecificStreamInfo = NULL; | ||
| inputParameters.device = devIndex; | ||
| if( nominal_sample_rate < 0 || Pa_IsFormatSupported( &inputParameters, NULL, nominal_sample_rate )) | ||
| return info->name; | ||
| debug("%d is not supported by dev %s \n", nominal_sample_rate, info->name); | ||
| } | ||
| return NULL; | ||
| } |
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.
This doesn't seem to be working well. Few things:
It returns NULL if the sample rate is supported, since Pa_IsFormatSupported() will returns a PaError and paNoError is 0. So if nominal_sample_rate is not less than zero, it appears to get the support backward.
Unless it is supposed to return NULL if the format is supported? It's good to write documentation when submitting code to a project, so that other's can understand it.
It requires a stereo microphone. My mics are mono, so they get rejected.
It doesn't work to query a device for format support while using the device at the same time with PortAudio's ALSA driver.
| #ifndef SRC_AUDIO_H_ | ||
| #define SRC_AUDIO_H_ | ||
|
|
||
| #include "tg.h" |
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.
But tg.h includes audio.h. Doesn't make sense have audio include tg and tg include audio.
| double phase = fmod(event, sweep) / sweep; // 0.0 -> 1.0 | ||
| double cycle = strip_width * ( 0.5 + (phase - 0.5) * zoom_factor); | ||
| int column = floor(fmod(cycle, strip_width)); |
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.
This zoom only works if sweep is a integer multiple of strip_width. If not, what happens is phase = fmod(event, sweep) will increase/decrease until it reaches zero and wraps around. At the same time, column = fmod(cycle, strip_width) is also wrapping around as the path cross the chart edges. If column doesn't wrap around at the same time as phase, then the path will jump from some point in the middle of the chart, by an amount equal to fmod(sweep, strip_width). The don't wrap at the same time if sweep isn't an exact multiple of strip_width.
In my PR that adds a smooth beat length zoom I wrote a totally new algorithm that solves this problem.
Signed-off-by: Jake Lewis <jakelewis3d@gmail.com>
Signed-off-by: Jake Lewis <jakelewis3d@gmail.com>
|
I've addressed most of the issues itemized above that relate to the features: Features have been implemented by others elsewhere, and can be ignored from this PR |
calculation of beat error moved from first pulse (unlock), to third pulse (lock)
removed conversion of beat error to an absolute number. Beat Error now shows + or -
|
update signed beat error. |




This PR replaces my previous PR #19 which can be ignored and closed.
1)audio interface and audio input selection. Light subsampling removed.
Under the command menu, the audio setting dialog allows user choice of audio interface and audio input. Default values work best. I'm not sure these should be exposed on release builds as some combinations do not produce a signal. For Windows though, using https://www.vb-audio.com/Cable/ is the easiest way to playback recordings, so it's needed for dev builds.
Sample rate can be selected too, but is not guaranteed to be available, again default values work best.
The subsampling of Light algorithm has been removed, as that can now be selected independently above. Light algorithm is now just the lowered buffer sizes without noise reduction.
2)removed round() methods from inner loops to outer loops in compute_phase and compute_waveform.
These were using 5% according to vtune. No change in output.
3)SIMD alignment of all fftw buffers.
There may be a reason for it, but some fftwf buffers were using malloc rather than the DWORD alingned memory that allows SIMD operations. http://www.fftw.org/doc/SIMD-alignment-and-fftw_005fmalloc.html#SIMD-alignment-and-fftw_005fmalloc Can't say I noticed any huge gains though.
4)cyan / magenta colored events a waveforms for tic / toc
Rather than all the events colored white, the tics and tocs are now colored cyan and magenta, as are the waveforms and period background. This makes it much easier to reduce beat errors as the colors switch places on the papertrace if you go too far. Off-centric escape wheels cycles are more evident too.
To maintain backward compatibility, this was done via tocs having negative eventtimes.
Methods
int absEventTime(int eventTime); int event_is_TIC_or_TOC(int eventTime);are used to convert timings back to regular time, and to tell if a time is a tic or toc.
5)Changed Lift Angle spinner to allow floating points. 0.5 increments.
Trivial change, but occasionally some lift angles are not integers.
6)Low and High Pass ranges added to Audio Settings Dialog
Added range controls to audio settings dialog so user can play with filter cutoffs. These values are saved between sessions. The order and effect of the filters has not been altered.