Waterfall fixes and improvements #510

Merged
merged 2 commits into from Feb 6, 2017

Projects

None yet

2 participants

@vsonnier
Collaborator
vsonnier commented Feb 5, 2017 edited

@cjcliffe My advantures in the inner workings of CubicSDR continues... This time, VisualProcessor and FFTDataDistributor. In FFTDataDistributor, I caught 2 bugs (see code annotations) one about loosing samples (BUGFIX1) and the other a frozen waterfall if conditions are met (BUGFIX2).

I've also added OPTION1 which consist in setting 2 times many waterfall lines, both in demod and main waterfall. This gives precise and pretty details, and do not seem to eat more CPU (tested on high end Core i7 mobile from 5 years ago with integrated graphics), and of course the main waterfall is 2 times slower for a given line-per-sec setting.
For this one I'll let you decide if you want it or not, so I'm not going to push this PR by myself.

@vsonnier vsonnier COMMENTS,POLISHING: comments fenzy around VisualProcessor machinery,
make process() a true interface as strong hint for derived classes,
plus misc define added for understanding.

BUGFIX: FFTDataDistributor loses incoming samples when compacting internal buffers.

BUGFIX2: FFTDistributor: Frozen Waterfall if internal buffer is no bigger than fftSize
4609386
#define DEFAULT_FFT_SIZE 2048
+#define DEFAULT_DMOD_FFT_SIZE (DEFAULT_FFT_SIZE / 2)
@vsonnier
vsonnier Feb 5, 2017 Collaborator

define all the things !

- bufferMax = inp->sampleRate / 4;
+
+ //bufferMax must be at least fftSize (+ margin), else the waterfall get frozen, because no longer updated.
+ bufferMax = std::max((size_t)(inp->sampleRate * FFT_DISTRIBUTOR_BUFFER_IN_SECONDS), (size_t)(1.2 * fftSize.load()));
@vsonnier
vsonnier Feb 5, 2017 Collaborator

BUGFIX2 part1 : if bufferMax < fftSize, waterfall get frozen in the following situation : a low sampling rate and big waterfall zoom which needs a big fftSize.

inputBuffer.sampleRate = inp->sampleRate;
inputBuffer.frequency = inp->frequency;
inputBuffer.data.resize(bufferMax);
}
+
+ //adjust (bufferMax ; inputBuffer.data) in case of FFT size change only.
+ if (bufferMax < (size_t)(1.2 * fftSize.load())) {
@vsonnier
vsonnier Feb 5, 2017 edited Collaborator

BUGFIX2 part2: inputBuffer.data must adapt to the changes of fftSize only.

+
+ //No room left in inputBuffer.data to accept inp->data.size() more samples.
+ //so make room by sliding left of bufferOffset, which is fine because
+ //those samples has already been processed.
if ((bufferOffset + bufferedItems + inp->data.size()) > bufferMax) {
memmove(&inputBuffer.data[0], &inputBuffer.data[bufferOffset], bufferedItems*sizeof(liquid_float_complex));
@vsonnier
vsonnier Feb 5, 2017 Collaborator

BUGFIX1: Previously, after the memmove (compacting) the memcpy was wrongly in a else branch, so no copying occured and the samples were lost.

- bufferedItems += inp->data.size();
+ //if there are too much samples, we may even overflow !
+ //as a fallback strategy, drop the last incomming new samples not fitting in inputBuffer.data.
+ if (bufferedItems + inp->data.size() > bufferMax) {
@vsonnier
vsonnier Feb 5, 2017 Collaborator

BUGFIX1 : after compacting, limit the copy to bufferMax anyway.

+ // derived class must implement a process() interface
+ //where typically 'input' data is consummed, procerssed, and then dispatched
+ //with distribute() to all 'outputs'.
+ virtual void process() = 0;
@vsonnier
vsonnier Feb 5, 2017 Collaborator

make process() a true interface to force concrete implementations in derived classes.

- // ratio required to achieve the desired rate
+ // ratio required to achieve the desired rate:
+ // it means we can achieive 'lineRateStep' times the target linesPerSecond.
+ // < 1 means we cannot reach it by lack of samples.
@vsonnier
vsonnier Feb 5, 2017 Collaborator

@cjcliffe My understanding of the principles:

  • if lineRateStep < 1, we are in sample famine, so consume all,
    else
  • Find a way to only distribute() at most the required line-per second rate, by eliminating some samples,

Question 1: Is it correct ? I don't understand the code below, I admit...
Question 2: Why not using a liquid resampler to adapt the number of samples ?

@cjcliffe
cjcliffe Feb 6, 2017 edited Owner

@vsonnier Ahoy,

  1. That sounds about right; it's main purpose was to grab the frame of samples that best lines up with the actual timing of the waterfall.
  2. Additional re-sampling seemed to create artifacts / aliases and requires more CPU; all the waterfall needs is a clean frame that's the correct size at the right time interval to keep things looking nice and from over/under flowing; the waterfall processor only re-samples using 1/2 decimation stages as needed and in-between it's just interpolating the data for visual display, which is much cheaper on the CPU + GPU than arbitrary re-sampling (which it used to do). To compensate for each 1/2 zoom level the waterfall is actually twice the resolution internally vs. what's displayed and when you cross that resolution boundary that's when the decimation kicks in another level (if you slow the spectrum way down and zoom quickly you can see it in action).
@vsonnier
vsonnier Feb 6, 2017 Collaborator

Thanks for the explanation 👍

+#define DEFAULT_SCOPE_FFT_SIZE (DEFAULT_FFT_SIZE / 2)
+
+//Both must be a power of 2 to prevent terrible OpenGL performance.
+#define DEFAULT_MAIN_WATERFALL_LINES_NB 1024
@cjcliffe
cjcliffe Feb 6, 2017 Owner

Going to knock this back down to 512; I like that it shows more but it's really flickery when you reduce the height a bit. I think we can make this configurable and even rig a 1:1 pixel match with multi-buffer scroll-back eventually.

+
+//Both must be a power of 2 to prevent terrible OpenGL performance.
+#define DEFAULT_MAIN_WATERFALL_LINES_NB 1024
+#define DEFAULT_DEMOD_WATERFALL_LINES_NB 256
@cjcliffe
cjcliffe Feb 6, 2017 Owner

This one looks better like this; it was always a bit stretched

@cjcliffe cjcliffe merged commit a161cf5 into cjcliffe:master Feb 6, 2017
@cjcliffe
Owner
cjcliffe commented Feb 6, 2017

All looks good; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment