gqrx icon indicating copy to clipboard operation
gqrx copied to clipboard

New rx_agc implementation

Open vladisslav2011 opened this issue 2 years ago • 21 comments

New AGC implementation with less complexity. CPU load does not vary depending on input. New controls, that should be easy to understand: Maximum gain - maximum AGC gain in auto mode. Target level - a level, to normalize the signal to in dB. Attack - attack time in ms. Time to go from max gain to 0 dB (worst case). Defines the size of the delay buffer and, as a result, sets audio delay, introduced by the block. Decay - decay time in ms. Time to go from 0 dB gain to maximum gain (worst case). Hang - hang time in ms. Time to wait before increasing the gain after the peak.

GUI and setting are updated.

Switch to float and move rx_agc to the output of demodulator. Works for FM modes. No more need to adjust gain while tuning between WFM stations.

Remove obsolete audio_gain# blocks

Move common code from nbrx/wfmrx to receiver_base Move all agc processing into block class rx_agc_2f and eliminate CAgc class Remove CurrentGainDb and move processing to get_current_gain Rename SetParameters to set_parameters Remove ProcessData and move all processing to work Fix tag propagation setting sample delay Workaroung GNU Radio < 3.8 tag propagation bug: manually calculate new tag offsets Small cleanup Set audio mute by disconnecting audio_snk Switch to using history to improve performance

Flowgraph structure (master): gqrx-master

Flowgraph structure (this PR): gqrx-rx_agc

Unfortunately github does not play well with SVG images. Right-click the image and choose "Save image as...", then open saved image to get it rendered correctly.

vladisslav2011 avatar Jan 10 '22 23:01 vladisslav2011

There are a lot of changes here. Unfortunately I don't think I'll have time to review such a large changeset soon. Is there any way to reduce the number of changes or split this into smaller pieces?

argilo avatar Jan 10 '22 23:01 argilo

Also, did you find out what the root cause of #1055 was? Is there a smaller fix we can apply in the interim, until there's time to review a major change to the AGC?

argilo avatar Jan 10 '22 23:01 argilo

Testing results. Squelch open: new_squelch_open Squelch closed: new_squelch_closed Quick test with airband IQ recording have shown good performance, no noticeable bugs (needs more testing) and predictable reaction to controls.

OK. CI failed. Apple clang does not like exp10f. Moving to draft.

vladisslav2011 avatar Jan 10 '22 23:01 vladisslav2011

Also, did you find out what the root cause of #1055 was?

This https://github.com/gqrx-sdr/gqrx/blob/master/src/dsp/agc_impl.cpp#L238-L243

But I do not like the overall implementation. Complex magnitude calculation for example: https://github.com/gqrx-sdr/gqrx/blob/master/src/dsp/agc_impl.cpp#L217-L220 - what the... I do not like it's placement too. It's pointless to put the AGC before FM demodulator. It is much better to put the AGC after the demodulator. It would adjust for deviation differences and make listening to NFM signals more comfortable too.

  • It would be possible to remove audio_gain# blocks as the AGC would prevent clipping if enabled and emulate gain blocks when disabled. Less blocks - less CPU usage, less CPU usage - higher input rate on RPi and similar weak devices.

vladisslav2011 avatar Jan 10 '22 23:01 vladisslav2011

Is there a reason not to just use one of the AGC blocks built into GNU Radio?

argilo avatar Jan 10 '22 23:01 argilo

I've replaced the exp10f with powf(10, . This should make clang happy. I'll reopen when CI ends.

vladisslav2011 avatar Jan 10 '22 23:01 vladisslav2011

Is there a reason not to just use one of the AGC blocks built into GNU Radio?

It may be possible. I have plans to test them too. Even make it possible to switch between different AGC implementations (not for upstream though). But there is no GNU Radio stereo AGC block... So, putting a GNU Radio block after the demodultor would either require interleaving stereo to sort of "IQ" and deinterleaving later and separate gain blocks for disabled mode. Or using 2 AGC blocks and synchronizing gains... Not possible. Interleaving/deinterleaving would increase CPU usage and introduce inaccuracies. Using 2 separate block would almost kill the stereo.

This PR is the 1st step to proper AGC. The second step would include making this block stereo 2in/2out and moving it after the demodulator. The audio_gain# blocks would be useless in this case and may be removed, switching the GUI control to manual_gain of the AGC block. There is special case with RAW IQ 'demodulator', that requires correct IQ magnitude calculation (as it is done now). This should be handled too.

All my PRs are small steps on the path to the attempt of #946 as I don't like LouDou's solution and want to try it myself. But I have to fix bugs first (iq_tool to make reproducible tests at high sample rates, reduce CPU usage to make it possible to use more demodulators at one in the future, etc.)

vladisslav2011 avatar Jan 10 '22 23:01 vladisslav2011

But there is no GNU Radio stereo AGC block...

I'm a bit confused. Why is stereo AGC needed?

argilo avatar Jan 11 '22 00:01 argilo

Different FM stations have different deviation. Where if WFM deviation setting? The AGC would scale all stations to full scale and prevent clipping. If somebody would be uncomfortable with this, then there is 'AGC Off' setting and manual gain to emulate current behavior.

vladisslav2011 avatar Jan 11 '22 00:01 vladisslav2011

Hmm, I'm not convinced this is a good idea. There are FM broadcast standards that define deviation. If we need to increase deviation in a bit in Gqrx to prevent clipping, then we should probably to do that instead. AGC will result in audio gain going up during silent periods, which seems undesirable.

argilo avatar Jan 11 '22 00:01 argilo

I do agree that AGC prior to FM demodulation is a waste of CPU cycles, so we could probably remove that.

argilo avatar Jan 11 '22 00:01 argilo

AGC will result in audio gain going up during silent periods, which seems undesirable.

There is hang setting to prevent this from happening. I know, that there are standards, but I see different stations broadcasting with different settings. Some use ~75kHz deviation, others use ~200kHz deviation... That's real world.

vladisslav2011 avatar Jan 11 '22 00:01 vladisslav2011

Where if WFM deviation setting?

If there are different standards in different parts of the world, we can add a setting for that. There is a setting for NBFM already.

argilo avatar Jan 11 '22 00:01 argilo

There is a setting for NBFM already.

This does not help when one correspondent on 2M uses motorola @25kHz deviation and other uses baofeng with 7kHz real deviation. It's hard to listen to the conversation...

vladisslav2011 avatar Jan 11 '22 00:01 vladisslav2011

OK, maybe there is some value in post-demod AGC for FM in an environment where nobody follows the rules. I haven't experienced that problem here.

argilo avatar Jan 11 '22 00:01 argilo

In my location some hams are always whispering far away from the microphone, some are always screaming and some are constantly switching between screaming and whispering... Funny to hear for the first time and not funny if you want to copy everything.

I'll do the second part... maybe tomorrow. Give it a try or leave it here until some free time would be available...

vladisslav2011 avatar Jan 11 '22 00:01 vladisslav2011

:+1:

Sounds good. I'll look at this when I get a chance. Gqrx is a spare-time project, so I don't always have a lot of time for review. That's why smaller changes are more likely to get reviewed, tested and merged quickly.

argilo avatar Jan 11 '22 00:01 argilo

I've made some bugfixes and adjustments. It is ready for testing. Suggestions and bug reports are welcome.

vladisslav2011 avatar Jan 14 '22 20:01 vladisslav2011

Some issues/bugs I've encountered with this change:

  • After unmuting, it takes around 2.5 seconds for audio to come back
  • Pressing mute while recording causes the recording to end, and the UI thinks the recording is ongoing when it actually ended
  • After pressing record, there's a 2-2.5 second delay before the recording starts, and audio playback also stops while waiting for recording to start. Audio playback also stops for 2.5 seconds when ending recording.

Tested on Fedora 38 and Mac OS 13.

sultanqasim avatar Aug 12 '23 16:08 sultanqasim

Thanks for testing. Some samples are lost due to GNU Radio regression https://github.com/gnuradio/gnuradio/issues/6105 So it should be fixed on GNU Radio side. It may be fixed by applying https://github.com/gnuradio/gnuradio/pull/6108/commits/3053725b8297e851f1c4b6e9b3c64f63e5a6c61a if GNU Radio is built from source...

vladisslav2011 avatar Aug 12 '23 19:08 vladisslav2011

I wish I understood the effects of making this change in GR better, wrt to tag propagation. I wonder if an optional param in lock/unlock unlock/restart would be useful in some way, to preserve the current behavior by default.

willcode avatar Aug 15 '23 12:08 willcode