Skip to content
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

Fix UDP streaming on MacOS, it was broken due to large UDP packets being truncated #1347

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

elafargue
Copy link
Contributor

#367 fixed an issue on MacOS tied to UDP packets being lost if packet size was too high.

A further change reintroduced the issue when adding support for GNURadio 3.10 (#982)

This PR fixes the issue but implementing the correct UDP packet size on MacOS.

Tested on digital modes, but a good way to verify the problem is to record UDP on FM radio, convert it with sox and listen: it is very clear audio is missing: audio is crackly and too fast.

nc -l -u 127.0.0.1 7355 | sox -t raw -c 1 -e signed-integer -b 16 -r 48000 - -t wav test_audio.wav

@argilo
Copy link
Member

argilo commented Apr 18, 2024

Thank you for your contribution.

Platform specific code is bug-prone, so I decided to look into the root cause of this issue more deeply to see if we can avoid it.

It looks like the root cause is not dropped packets (as was suspected in #363) but rather that some versions of netcat truncate UDP packets. The macOS version appears to truncate to 1024 bytes. I'll have a look into what other versions of netcat do, but maybe a good approach would be for Gqrx to limit UDP packets to 1024 bytes always.

@argilo
Copy link
Member

argilo commented Apr 18, 2024

The original netcat (version 1.10 from 1996) uses an 8192-byte buffer, so there's no issue.

GNU netcat (version 0.7.1 from 2004) uses a 1024-byte buffer. As far as I can tell, macOS uses a variant of GNU netcat. Some Linux distributions offer GNU netcat packages.

OpenBSD netcat used to have a 1024-byte buffer, but increased it to 2048 in 2010. Many Linux distributions seem to use OpenBSD netcat by default.

macOS netcat was forked from OpenBSD netcat prior to the buffer size increase, and it still uses a 1024-byte buffer today.

Copy link
Member

@argilo argilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the problem lies with certain implementations of netcat, which may be present on operating systems other than macOS, I think a better approach would be to remove the GQRX_OS_MACX macro completely (including from src/CMakeLists.txt) and set the maximum UDP packet size to 1024 everywhere.

If you'd like to update this pull request, you're welcome to. Or if not, I can open another one with that change.

@elafargue
Copy link
Contributor Author

Thanks for looking into it! I'll update the PR accordingly

@argilo
Copy link
Member

argilo commented Apr 18, 2024

Thanks!

Have you had a chance to test the change on your system? I'll try it on Linux as well (including with GNU netcat) and get this merged shortly.

@elafargue
Copy link
Contributor Author

Yes, tested on MacOS 14.4.1 on an Intel Mac, a 1024 byte buffer works fine. Validated via audio test (FM radio makes it super obvious when data is dropped, sound is too fast and crackly), plus digital modes w/ external demodulator piped via nc, all work ok.

@argilo
Copy link
Member

argilo commented Apr 18, 2024

Sounds great. I just tested on Linux with the original netcat, GNU netcat, and OpenBSD netcat. All are working well.

@argilo argilo changed the title Fix UDP streaming on MacOS, it was broken due to large UDP packets being lost Fix UDP streaming on MacOS, it was broken due to large UDP packets being truncated Apr 18, 2024
@argilo argilo merged commit 702ef6b into gqrx-sdr:master Apr 18, 2024
17 checks passed
grappas added a commit to neural75/gqrx-scanner that referenced this pull request Apr 21, 2024
@grappas
Copy link

grappas commented Apr 21, 2024

I request further notification, if you commit any changes for packet sizes again :)
BTW - standard packet size for GNU Radio UDP signal is 1472 bytes and I'm not sure if that change was that necessary.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

I request further notification, if you commit any changes for packet sizes again :)

Hyrum's Law? 😄

I'm not sure if that change was that necessary.

Gqrx is quite commonly used with netcat, so I think it is useful to avoid behaviour that is incompatible with it.

@grappas
Copy link

grappas commented Apr 21, 2024

I was testing it with socat. Worked without any hiccups. Netcat never worked for me with gqrx anyway, so commonly is an overstatement xD.
I would contact gnu radio maintainers why they made it 1472 in the first place.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

By the way, I had a peek at the pull request you are working on, and I wouldn't recommend dropping the buffer size to 1024, since that would be incompatible with earlier versions of Gqrx.

@grappas
Copy link

grappas commented Apr 21, 2024

By the way, I had a peek at the pull request you are working on, and I wouldn't recommend dropping the buffer size to 1024, since that would be incompatible with earlier versions of Gqrx.

It have to be exact for it to work properly.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

Why is that? Gqrx makes no guarantee about UDP packet size, and it can vary from packet to packet.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

It may be that packet length doesn't often vary in practice anymore due to #1271, but that's just an implementation detail that could change in the future.

@grappas
Copy link

grappas commented Apr 21, 2024

By the way, I had a peek at the pull request you are working on, and I wouldn't recommend dropping the buffer size to 1024, since that would be incompatible with earlier versions of Gqrx.

neural75/gqrx-scanner@cbd2637
https://media2.giphy.com/media/v1.Y2lkPTc5MGI3NjExZW52angzMXgwZmhhcTFhOTVxeWl4M2w4NjhhOXE0a24xbHd5eDFxeSZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/Mp4hQy51LjY6A/giphy.gif

@grappas
Copy link

grappas commented Apr 21, 2024

Why is that? Gqrx makes no guarantee about UDP packet size, and it can vary from packet to packet.

I'm pretty sure that packet size is constant and rest of stream is filled with \0 if rest is empty.

If rx buffer is too big it will wait till timeout (we don't want to wait). If too small rx will be messy and pretty much overshoot every time.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

I'm pretty sure that packet size is constant and rest of stream is filled with \0 if rest is empty.

Nope. Try with Gqrx 2.16, for example, and you will see that UDP packet sizes are all over the place:

154
204
192
4
20
4
192
14
208
10
192
14
96
328
192
128
256

You need to use the return value of recvfrom to determine how many bytes were in the UDP packet.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

It is pure coincidence that (most) UDP packets produced by Gqrx 2.17 and later are the same size, and this is in no way guaranteed. If a consuming application needs larger chunks, or chunks of an exact size, it must implement its own buffering to reassemble them.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

Gqrx 2.17.5 requests a maximum UDP packet size of 1024, but it is up to the GNU Radio scheduler to decide how many bytes go into each UDP packet, up to that limit. So behaviour may also vary depending on the GNU Radio version. The GNU Radio scheduler is not deterministic (since it depends on the order in which the operating system schedules thread execution), which is why the UDP packet lengths with Gqrx 2.16 and earlier are quite random.

@grappas
Copy link

grappas commented Apr 21, 2024

And imagine, that all that concerns goes away with #1341 ( ͡° ͜ʖ ͡°)
It would make UDP listening in that scanner obsolete in a hurry.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

Agreed, listening to the demodulator output is an inefficient way to scan.

@argilo
Copy link
Member

argilo commented Apr 21, 2024

By the way, you may want to look at Phil Karn's ka9q-radio, an experimental multi-channel receiver. It may be a better option than bolting a scanning program onto Gqrx, which is inherently a single-channel receiver.

@vladisslav2011
Copy link
Contributor

There are 2 PRs that convert Gqrx to a multichannel receiver: #997 and #1106 . You may try one or another. Or you may just check my fork, that features very efficient FFT channelizer...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants