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

DeviceInput::applySettings should be on a different thread? #1522

Open
srcejon opened this issue Nov 24, 2022 · 18 comments
Open

DeviceInput::applySettings should be on a different thread? #1522

srcejon opened this issue Nov 24, 2022 · 18 comments
Assignees

Comments

@srcejon
Copy link
Collaborator

srcejon commented Nov 24, 2022

It appears currently that input device's applySettings method executes on Qt's main thread. This can be problematic when updating an SDR setting that takes a long time (E.g. changing frequency can take ~100ms on some devices), as it can block the Qt UI and cause audio drop outs on other SDRs that are running.

To see this, create a workspace with two SDRs. Set one up to demod broadcast FM. For the other SDR, add a delay in to applySettings, to accentuate the problem. E.g.:

QThread::sleep(1);
// just before rtlsdr_set_center_freq for example

Now when you change the frequency on the second SDR, you should hear an audio drop out for the first SDR and the display will freeze.

Obviously no devices take as long as this, but I've heard a few brief drop outs in normal operation, and I think they are related to this. Just thinking theoretically, if the UI is blocked for >20ms, that would mean a video frame could be missed.

@srcejon
Copy link
Collaborator Author

srcejon commented Nov 25, 2022

Another possible example of this, is if you have one SDR demoding audio, then you press start on another device, there can be a brief audio drop-out.

@f4exb
Copy link
Owner

f4exb commented Nov 25, 2022

Maybe before that we should fix heavy calculations handled in the GUI of some components that happen on the main thread.

Also regarding Aaudio maybe it is the audio that should be handled on a different thread.

@f4exb
Copy link
Owner

f4exb commented Nov 25, 2022

if the UI is blocked for >20ms, that would mean a video frame could be missed.

If it is just that I think there is nothing to worry about. For any operation that would be critical it is normal to limit the setup to the strict necessary so interference from unrelated devices or components is irrelevant. Spawning a new thread systematically just for the applySettings is not worth the hassle. Only very noticeably lengthy processes not just limited to the scope of aaplySettingsare worth thinking about putting them in a separate thread.

@srcejon
Copy link
Collaborator Author

srcejon commented Nov 25, 2022

Maybe before that we should fix heavy calculations handled in the GUI of some components that happen on the main thread.

What did you have in mind? The basic spectrum seems to run easily at 50+ FPS, so I presume it takes less time to run than applySettings (but I'm using a relatively fast PC).

Also regarding audio maybe it is the audio that should be handled on a different thread.

It's not just audio though - the entire UI is unresponsive. So, for example, the spectrum stops updating.

@f4exb
Copy link
Owner

f4exb commented Nov 25, 2022

I cannot use the Map feature at all on Ubuntu 22.04 when some updates are coming in, It does not seem to be as bad on Windows though so there might be something else fishy there,,,

@srcejon
Copy link
Collaborator Author

srcejon commented Nov 25, 2022

I cannot use the Map feature at all on Ubuntu 22.04 when some updates are coming in

What do you mean by updates? When it's updating the ionosphere propagation data or something like that (It updates every 15 minutes)? I haven't seen a problem, but haven't really been looking for one, so will take a look.

@f4exb
Copy link
Owner

f4exb commented Nov 25, 2022

AIS for example each time a new message comes in:

map-2022-11-25_22.28.27.mp4

@srcejon
Copy link
Collaborator Author

srcejon commented Nov 25, 2022

Ok - definitely a problem there, so will take a look.

I've been working on one performance improvement that improves the anti-aliasing and clips off screen objects, as it appears Location doesn't do that for you - but that's probably not the problem here.

@srcejon
Copy link
Collaborator Author

srcejon commented Nov 26, 2022

Do you have the 3D map disabled (in settings dialog) or is it just hidden using the splitter?

@f4exb
Copy link
Owner

f4exb commented Nov 26, 2022

Do you have the 3D map disabled (in settings dialog) or is it just hidden using the splitter?

Indeed in this case the 3D map is disabled. I would normally enable it only if necessary or if I fancy it just to be more conservative on CPU. However I did notice that if I disable 2D and enable 3D there is no more freezing issues although the CPU load goes higher.

I also investigated a bit more on the original subject. One case that can be really annoying is changing sample rate (and it depends somehow on the sample rate value) on the Lime SDR. This can freeze everything for at least one second. It seems the LimeSuite library does things synchronously while some other libraries may do it asynchronously. So I would say that this is pretty much case dependent and if we would like to turn to asynchronous this would have to be done in the library wrapper (there is one for a number of devices in the https://github.com/f4exb/sdrangel/tree/master/devices folder). I don't think it is reasonable to run the applySettings method on its own thread as it can raise many more issues than it is supposed to solve. Multithreading in particular in Qt has to be used cautiously as reminded in the documentation: https://doc.qt.io/qt-6/thread-basics.html#when-to-use-alternatives-to-threads

@f4exb
Copy link
Owner

f4exb commented Nov 26, 2022

In addition to the map subject: I also noticed that the embedded map in the ADSB demodulator does not exhibit the issue.

Also to refine things a bit: even with the 3D map enabled there is still an issue if the 2D is enabled. One other thing I noticed is that disabling ground tracks has a very positive effect. There is no more noticeable freezing issues.

@f4exb f4exb self-assigned this Dec 26, 2022
@srcejon
Copy link
Collaborator Author

srcejon commented Jan 1, 2023

Also regarding audio maybe it is the audio that should be handled on a different thread.

This might be a good idea. I've noticed some other occasional audio drop outs on Windows, when doing things like resizing the window or zooming in to the spectrum.

@srcejon
Copy link
Collaborator Author

srcejon commented Jan 17, 2023

I cannot use the Map feature at all on Ubuntu 22.04 when some updates are coming in, It does not seem to be as bad on Windows though so there might be something else fishy there,,,

I've just tried reproducing what you see in your video, but I don't see it, on either Windows or Linux. For Linux, I'm running a VM with a single CPU, to try to reduce available processing power. I don't have a Persues SDR though. Does it happen with other SDRs?

What does top report for your CPU usage? I can see/hear stuttering if I run other processes, in order to get CPU usage up to 100%. But that's not really a threading issue.

@f4exb
Copy link
Owner

f4exb commented Feb 1, 2023

Another strange thing I noticed is that when the 2D map from MapboxGL is displayed on the VOR Localizer it updates much faster when zomming in/out and moving that with the same kind of map on the Map feature where it is not very fluid even with no artifacts displayed on top of the map (disabling all of them). Same difference with the ADSB demod.

@srcejon
Copy link
Collaborator Author

srcejon commented Feb 1, 2023

Another strange thing I noticed is that when the 2D map from MapboxGL is displayed on the VOR Localizer it updates much faster when zomming in/out and moving that with the same kind of map on the Map feature where it is not very fluid even with no artifacts displayed on top of the map (disabling all of them). Same difference with the ADSB demod.

When disabled, they aren't completely removed from the map, just hidden, which possibly explains it. I have been working on some improvements to this, which are nearly ready. Got side tracked with the Heat Map!

@f4exb
Copy link
Owner

f4exb commented Feb 27, 2023

The latest version (7.10.0) of the Map feature shows great improvements in the 2D map experience. It is as fluid as the map on VOR localizer or ADSB unless there are a lot of artifacts to display like displaying all beacons at continent level (so really a lot).

@srcejon
Copy link
Collaborator Author

srcejon commented Feb 27, 2023

Good. There's now also a filter by distance column in the settings table, you can use to prevent display of items that are far away.

Do you still see the problem you were seeing with AIS in the video above?

@srcejon
Copy link
Collaborator Author

srcejon commented Jun 30, 2023

#1717 has eliminated the audio problems this causes - but there can still be a pause in the spectrum / other UI elements.

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

No branches or pull requests

2 participants