gqrx icon indicating copy to clipboard operation
gqrx copied to clipboard

Implement audio sample rate selection

Open vladisslav2011 opened this issue 2 years ago • 20 comments

Close #615 Includes https://github.com/gqrx-sdr/gqrx/pull/1036, that closes #1030 and #1018.

vladisslav2011 avatar Dec 28 '21 18:12 vladisslav2011

Could you please limit this pull request to just the audio rate selection? As I've mentioned previously, combining multiple changes into a single pull request makes it difficult to review.

argilo avatar Dec 30 '21 04:12 argilo

Could you please limit this pull request to just the audio rate selection?

Making audio rate selection work properly automatically closes #1030 and #1018. I can split bug fixes into separate PR and make this PR depend on new one.

vladisslav2011 avatar Dec 30 '21 10:12 vladisslav2011

I noticed a bug: the frequency axis on the audio FFT seems to be incorrect when lower audio sample rates are used.

argilo avatar Dec 30 '21 22:12 argilo

Thanks for a review.

the frequency axis on the audio FFT seems to be incorrect

Confirmed. It may be fixed by setting correct sampling rate on receiver::audio_fft. Wait some minutes, I'll check it and reopen the PR.

vladisslav2011 avatar Dec 30 '21 22:12 vladisslav2011

Another problem: filter widths are not restricted, and so aliasing can now occur when the audio rate is decreased. For instance, when I widen the USB filter, a tone that is outside the displayed filter skirts can be heard:

Screenshot from 2021-12-30 17-27-14

argilo avatar Dec 30 '21 22:12 argilo

This is because the demod ranges defined in selectDemod are built under the assumption that the audio rate will be 48000. For instance:

https://github.com/gqrx-sdr/gqrx/blob/7b736c41f150a2741f6a30d81d190c076e5e5003/src/applications/gqrx/mainwindow.cpp#L1157

argilo avatar Dec 30 '21 22:12 argilo

It looks like this will need some careful consideration to account for all the places in the UI and DSP that are making assumptions about the audio rate.

argilo avatar Dec 30 '21 22:12 argilo

For instance, when I widen the USB filter, a tone that is outside the displayed filter skirts can be heard:

That's just filter leakage. It can be heard at 48000Hz rate too. Setting the filter to 'sharp' reduces the peak and tone, setting the filter to 'soft' increases it. I see no aliasing even when the peak is in filter range: 2021-12-31 02-19-43 Audio is resampled always correctly with filtering applied before decimation. https://github.com/gqrx-sdr/gqrx/blob/master/src/dsp/resampler_xx.cpp#L52-L60

On one hand, limiting the maximum filter width (AM, SSB, CW, RAW IQ modes) to a value, that will always fit into output bandwidth is a good idea, but on the other hand, altering user settings in unexpected way may be bad. In case of FM the bandwidth includes the deviation, so in case of WFM it can be very wide even when modulating signal bandwidth is narrow. So, we should not limit the filter width in FM modes. I've fixed the bug with incorrect frequency axis on the audio FFT. Reopening.

vladisslav2011 avatar Dec 30 '21 23:12 vladisslav2011

That's just filter leakage.

Ah, I think you're right. After looking closer, I see that nbrx and wfmrx have audio resamplers late in their receive chains, which should prevent aliasing. My suggestion to limit the demod ranges was based on that incorrect assumption, so I think it's fine to leave them as they are.

I'll still need to do a bit more testing to make sure that there aren't any other bugs.

argilo avatar Dec 31 '21 01:12 argilo

I see now why I was confused by the behaviour when I expanded the filter bandwidth. The filter's transition width depends on the low & high filter cutoffs, so it becomes very large when the filter is expanded:

https://github.com/gqrx-sdr/gqrx/blob/7b736c41f150a2741f6a30d81d190c076e5e5003/src/applications/gqrx/receiver.cpp#L678-L705

This is surprising. I would have expected the transition width to depend only on the mode and the filter shape.

argilo avatar Dec 31 '21 01:12 argilo

Another issue. While in 8 kHz mode, switching from USB to FM (stereo) leaves the audio FFT in a weird configuration where negative frequencies are displayed:

Screenshot from 2021-12-31 00-47-29

argilo avatar Dec 31 '21 05:12 argilo

Maybe that happens because the requested range exceeds what is possible given the current audio sample rate. There's even a FIXME about that:

https://github.com/gqrx-sdr/gqrx/blob/4eaeb163ac470d187d928a71622fb5dbdaedaa9e/src/applications/gqrx/mainwindow.cpp#L1132

Many other modes similarly request FFT ranges that may not be possible.

argilo avatar Dec 31 '21 05:12 argilo

I guess all the ranges should be capped to a maximum of half the audio rate.

argilo avatar Dec 31 '21 06:12 argilo

I guess all the ranges should be capped to a maximum of half the audio rate.

Thanks for suggestion. This works very well.

vladisslav2011 avatar Jan 03 '22 20:01 vladisslav2011

Rebased on top of current master. It was quite hard. The previous "recreate the demod from scratch" approach was much, much easier. I was hit by https://github.com/gnuradio/gnuradio/issues/5436 several times during rebase, so this PR needs some testing to ensure, that it is free from regressions...

I have added 88200 Hz and 96000 Hz audio rates (mostly for "RAW IQ" mode).

vladisslav2011 avatar Jan 11 '22 20:01 vladisslav2011

Rebased on top of current master. And closed by accident. How can I reopen this PR? Pushing new commits, as github suggests, does not help...

vladisslav2011 avatar Jan 13 '22 16:01 vladisslav2011

OK. Pushing a new commit has solved the issue.

vladisslav2011 avatar Jan 13 '22 16:01 vladisslav2011

Sorry, it looks like I caused a merge conflict when I merged #1066.

I just had a quick look over this code and it's looking decent. I think I'll try to including this in the next feature release (2.16) since it's a new feature.

argilo avatar Jan 15 '22 19:01 argilo

I caused a merge conflict

Resolved.

vladisslav2011 avatar Jan 15 '22 19:01 vladisslav2011

Aside from my nitpick regarding the double precision argument to the set_rate function in fm_deemph.h, this patch is working very well for me and I have not noticed any regressions in my testing. I would support this being merged once this the warning causing double precision nitpick is fixed.

sultanqasim avatar Aug 12 '23 16:08 sultanqasim