Setting the filter width too narrow causes DSP to hang

[AsciiWolf]

Fedora 38 with gqrx-2.15.9-7.fc38.x86_64. (However, I was able to reproduce the same issue with latest Gqrx master.)

DSP hangs after setting the filter width too narrow using Ctrl + mouse wheel. See the following screencast:

filter_width_hang.webm

This gets printed on stdout:

block_executor :error: sched: <block fir_filter_blk<IN_T,OUT_T,TAP_T> (14)> is requesting more input data  than we can provide.  ninput_items_required = 11570  max_possible_items_available = 8191  If this is a filter, consider reducing the number of taps.

[vladisslav2011]

I can confirm this issue.
Another way to reproduce:

1. Set "Filter shape" to "Soft"
2. Set "Filter width" to "Wide"
3. Set "Mode" to CW-L/CW-H
4. Set "Filter width" to "Narrow"
5. Set "Filter shape" to "Sharp"

DSP freezes and GNU Radio complains about insufficient data in a buffer:

sched: <block fir_filter_ccc (221)> is requesting more input data
  than we can provide.
  ninput_items_required = 11570
  max_possible_items_available = 8191
  If this is a filter, consider reducing the number of taps.

[argilo]

I think I've seen this before. One contributing factor is that the filter transition width is calculated from the filter bandwidth, so the filter becomes sharper (and therefore requires more taps) as the filter becomes narrower:

gqrx/src/applications/gqrx/receiver.cpp

Lines 687 to 714 in aa151a2

	receiver::status receiver::set_filter(double low, double high, filter_shape shape)
	{
	double trans_width;
	if ((low >= high) || (std::abs(high-low) < RX_FILTER_MIN_WIDTH))
	return STATUS_ERROR;
	switch (shape) {
	case FILTER_SHAPE_SOFT:
	trans_width = std::abs(high - low) * 0.5;
	break;
	case FILTER_SHAPE_SHARP:
	trans_width = std::abs(high - low) * 0.1;
	break;
	case FILTER_SHAPE_NORMAL:
	default:
	trans_width = std::abs(high - low) * 0.2;
	break;
	}
	rx->set_filter(low, high, trans_width);
	return STATUS_OK;
	}

It might make sense to make the transition width constant.

Otherwise, we'd need to set a lower limit on the transition width, or make sure the buffer size is large enough to accommodate long filters.

[vladisslav2011]

It looks like another GNU Radio bug.
fir_filter_blk_impl sets history inside of work, that should never happen.
https://github.com/gnuradio/gnuradio/blob/main/gr-filter/lib/fir_filter_blk_impl.cc#L72
Setting history may require buffer reallocation. So it should be either set once in the constructor and never changed or changed in the 'locked' flowgraph state.
fir_filter_blk_impl<IN_T, OUT_T, TAP_T>::set_taps may be changed to check that there is enough history to call d_fir.filterN with new taps and either return false or throw an exception.
In this case it will be possible to reserve some buffer space by constructing the block with large initial number of taps or explicitly setting the history to a maximum number of taps (+1?) before starting the flowgraph.

[vladisslav2011]

Possible fix: 6387148

[argilo]

I was curious whether this bug is old, and it's been around since at least version 2.6.

In this PR I tried to improve the situation: #825

But that didn't solve the problem for the "sharp" filter mode, and I also now see that it didn't limit the filter width when Ctrl+mouse wheel is used. (The bandwidth can even go negative when the mouse wheel is used!)

Possible fix: 6387148

I'd like to avoid duplicating GNU Radio functionality in Gqrx if we can. I'll have a look around to see if there are other options.

I did a quick test lowering the transition width here:

gqrx/src/receivers/nbrx.cpp

Line 46 in aa151a2

filter = make_rx_filter(PREF_QUAD_RATE, -5000.0, 5000.0, 1000.0);

But unfortunately that only forces the buffer size to be larger until the next time the flow graph is reconfigured.

[argilo]

It looks like another GNU Radio bug.

It's probably worth opening an issue to see whether something can be done upstream.

[willcode]

Since buffers live on the output side of each block in GR, and FIRs need history on the input, there isn't a whole lot that can be done about this in GR 3.X, unless new functionality is added to reallocate buffers on the fly. The workaround is simple enough, even though it seems like the user program shouldn't have to have this kind of understanding of GR.

To make things a little more logical, we could add a reserve_min_input() kind of function to blocks, which would have the same effect as set_min_() on the preceding block. This would be better for reconfiguration too, when the preceding block isn't always the same.

[vladisslav2011]

There is already set_history(unsigned history) that does exactly the same thing: it reserves some space in upstream block's buffer.

[willcode]

Ha, good point. That should work too.

[vladisslav2011]

It works only when narrow filter with sharp rolloff is selected at the time of nbrx creation. If the filter is changed to Normal/Wide /Soft/Normal and the DSP is restarted (tb->stop()/tb->start() or tb->lock()/tb->unlock()) the history gets reset to the smaller value and changing the filter to sharp/narrow locks the DSP again.
https://github.com/gnuradio/gnuradio/blob/main/gr-filter/lib/fir_filter_blk_impl.cc#L72

[willcode]

set_history() isn't going to work in this case - too much interaction with the filter code, and blocks don't have set_history() as a public method. So, the fix I put up is the only (simple) think I can thing of at the moment.

[vladisslav2011]

blocks don't have set_history()

Hmm.... What version of GNU Radio are you using?
set_history() is public here:
https://github.com/gnuradio/gnuradio/blob/main/gnuradio-runtime/include/gnuradio/block.h#L94

[willcode]

Don't know what I was looking at - so this works fine too looked like it was going to work:

diff --git a/src/dsp/rx_filter.cpp b/src/dsp/rx_filter.cpp
index 2c5b1185..6f83e36a 100644
--- a/src/dsp/rx_filter.cpp
+++ b/src/dsp/rx_filter.cpp
@@ -62,6 +62,7 @@ rx_filter::rx_filter(double sample_rate, double low, double high, double trans_w
 
     /* create band pass filter */
     d_bpf = gr::filter::fir_filter_ccc::make(1, d_taps);
+    d_bpf->set_history(64 * 1024);
 
     /* connect filter */
     connect(self(), 0, d_bpf, 0);

[vladisslav2011]

No. I've tried everything...
It looks working first, but after switching to AM/NFM the bug is here again...

[willcode]

The max output buffer set to 8192 instead of requested 65536 thing does seem unnecessary. I'll kill that PR - need to read through the buffer alloc code some more.

[vladisslav2011]

There are 2 different problems:

1. flat_flowgraph::allocate_block_detail ignores min_output_buffer setting
2. fir_filter_blk_impl sets history from work, that can't work immediately and breaks things when the flowgraph is stopped/started.

[willcode]

If anyone gets a chance to retest this with the fix in GNU Radio main, let me know how it works. I think this solve this one problem, though the general problem of changing history remains. I think we can put this change into GR 3.10.7.0.

[willcode]

That fix is now in GR v3.10.7.0.