-
Notifications
You must be signed in to change notification settings - Fork 319
Fix WPF inference UI thread blocking and add busy indicator #619
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
1c4ec6e
b4dbf24
181695a
2d48d1f
1d931fa
40da524
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -182,6 +182,14 @@ private void SelectImageButton_Click(object sender, RoutedEventArgs e) | |||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private void SetBusy(bool busy) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| BusyIndicator.Visibility = busy ? Visibility.Visible : Visibility.Collapsed; | ||||||||||||||||||||||||||||||||
| RunInferenceButton.IsEnabled = !busy && _session != null && !string.IsNullOrEmpty(_selectedImagePath); | ||||||||||||||||||||||||||||||||
| ReloadSessionButton.IsEnabled = !busy; | ||||||||||||||||||||||||||||||||
| SelectImageButton.IsEnabled = !busy; | ||||||||||||||||||||||||||||||||
|
Comment on lines
+203
to
+206
|
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+200
to
+226
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| private async void RunInferenceButton_Click(object sender, RoutedEventArgs e) | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| if (string.IsNullOrEmpty(_selectedImagePath) || _session == null) | ||||||||||||||||||||||||||||||||
|
|
@@ -192,13 +200,13 @@ private async void RunInferenceButton_Click(object sender, RoutedEventArgs e) | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| SetBusy(true); | ||||||||||||||||||||||||||||||||
| ResultsTextBox.Text = "Running inference..."; | ||||||||||||||||||||||||||||||||
| Dispatcher.Invoke(() => { }, System.Windows.Threading.DispatcherPriority.Render); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); | |
| using var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); |
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task.Run(() => InferenceEngine.RunInference(_session, ...)) captures the _session field and runs inference on a background thread, but MainWindow_Closed calls Dispose() which disposes _session without coordinating with any in-flight inference task. Closing the window during inference can therefore dispose the session while session.Run(...) is executing, which can lead to crashes or undefined behavior (native resources). Consider capturing _session into a local variable before starting the task and coordinating shutdown (e.g., track/await the inference task on close, or prevent disposal until the background work completes / is canceled).
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RunInferenceButton_Click captures _session into a local session for Task.Run, but then calls ExtractResults(_session, results) using the field again. If _session changes (or is disposed) across the awaited calls, results extraction can use a different session’s metadata or hit a null/disposed session. Use the same captured non-null session for both RunInference and ExtractResults, and capture it before the first await in the method to keep it consistent for the whole inference operation.
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetBusy(true) disables DeviceCombo, but LoadModelAndLabelsAsync uses DeviceCombo.IsEnabled to decide whether to read DeviceCombo.SelectedItem (and whether to require a device selection). When ReloadSessionButton_Click calls SetBusy(true) before LoadModelAndLabelsAsync, the chosen device type will always be treated as null, which can change EP/device selection behavior. Consider capturing the EP/device selection before calling SetBusy(true), or update LoadModelAndLabelsAsync to use a separate “device selection required” signal (e.g., based on available device types / item count) rather than IsEnabled, since IsEnabled is now also used for the busy UI state.
| { | |
| SetBusy(true); | |
| { | |
| var deviceComboWasEnabled = DeviceCombo.IsEnabled; | |
| var selectedDevice = DeviceCombo.SelectedItem; | |
| SetBusy(true); | |
| // Preserve the user's device-selection state for model loading. | |
| // LoadModelAndLabelsAsync uses DeviceCombo.IsEnabled/SelectedItem to decide | |
| // whether a device selection is required and which device to use. SetBusy(true) | |
| // temporarily disables the combo for UI purposes, so restore the pre-busy state | |
| // before loading to avoid changing EP/device selection behavior. | |
| DeviceCombo.IsEnabled = deviceComboWasEnabled; | |
| DeviceCombo.SelectedItem = selectedDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetBusy()disables the three buttons but leaves other interactive controls enabled (e.g.,EpCombo,DeviceCombo,AllowProviderDownloadCheckBox). BecauseLoadModelAndLabelsAsync()reads these values afterawaitpoints, the user can change selections mid-load and end up creating a session with a different EP/device than the one they clicked with. Consider disabling those inputs while busy (or snapshotting their values before any awaits) so model load is deterministic.