Fix WPF inference UI thread blocking and add busy indicator#619
Fix WPF inference UI thread blocking and add busy indicator#619yeelam-gordon wants to merge 6 commits intomainfrom
Conversation
RunInference was blocking the UI thread, risking freeze on larger models. Fixes ADO #61791035 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…el loading Show a visual progress bar and disable interactive buttons while async operations (inference, model reload) are in progress. Replaces the Dispatcher.Invoke render-flush workaround with proper busy-state management. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the WindowsML WPF sample to avoid UI freezes during inference by running the synchronous ONNX Runtime session.Run(...) work off the UI thread, and adds a busy indicator to communicate/guard “work in progress” states.
Changes:
- Add a
SetBusy(bool)helper to show an indeterminate progress bar and disable user actions during long-running work. - Offload
InferenceEngine.RunInference(...)to a background thread viaTask.Run(...). - Wrap model reload in
try/catch/finallyto reliably clear the busy state and surface errors in the results textbox.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Samples/WindowsML/cs-wpf/MainWindow.xaml.cs | Adds SetBusy, moves inference to Task.Run, and adds busy-state handling around model reload/inference. |
| Samples/WindowsML/cs-wpf/MainWindow.xaml | Adds a hidden-by-default indeterminate ProgressBar used as the busy indicator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BusyIndicator.Visibility = busy ? Visibility.Visible : Visibility.Collapsed; | ||
| RunInferenceButton.IsEnabled = !busy && _session != null && !string.IsNullOrEmpty(_selectedImagePath); | ||
| ReloadSessionButton.IsEnabled = !busy; | ||
| SelectImageButton.IsEnabled = !busy; |
There was a problem hiding this comment.
SetBusy() disables the three buttons but leaves other interactive controls enabled (e.g., EpCombo, DeviceCombo, AllowProviderDownloadCheckBox). Because LoadModelAndLabelsAsync() reads these values after await points, 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.
| SelectImageButton.IsEnabled = !busy; | |
| SelectImageButton.IsEnabled = !busy; | |
| EpCombo.IsEnabled = !busy; | |
| DeviceCombo.IsEnabled = !busy; | |
| AllowProviderDownloadCheckBox.IsEnabled = !busy; |
| BusyIndicator.Visibility = busy ? Visibility.Visible : Visibility.Collapsed; | ||
| RunInferenceButton.IsEnabled = !busy && _session != null && !string.IsNullOrEmpty(_selectedImagePath); | ||
| ReloadSessionButton.IsEnabled = !busy; | ||
| SelectImageButton.IsEnabled = !busy; |
There was a problem hiding this comment.
SetBusy() computes RunInferenceButton.IsEnabled based on _session and _selectedImagePath, but SelectImageButton_Click elsewhere still sets RunInferenceButton.IsEnabled = true unconditionally. That can re-enable the button when _session is null (model not loaded) and undermines the new centralized enable/disable logic. Consider updating the image-selection path to call a single helper (e.g., SetBusy(false) / UpdateUiState()) instead of directly toggling RunInferenceButton.IsEnabled so the enabled state is consistent everywhere.
|
|
||
| using var results = InferenceEngine.RunInference(_session, inputTensor); | ||
| using var results = await Task.Run(() => InferenceEngine.RunInference(_session, inputTensor)); | ||
| var resultTensor = InferenceEngine.ExtractResults(_session, results); |
There was a problem hiding this comment.
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).
…state, capture session local Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| RunInferenceButton.IsEnabled = true; | ||
| RunInferenceButton.IsEnabled = _session != null; | ||
| ResultsTextBox.Text = "Image selected. Click 'Run Inference' to classify the image."; |
There was a problem hiding this comment.
SelectImageButton_Click now disables the Run Inference button when _session is null, but the status text still says "Click 'Run Inference'" even when inference can’t be run yet. Update the message to reflect whether the model is loaded (e.g., prompt to load/reload the model first when _session is null).
| ResultsTextBox.Text = "Image selected. Click 'Run Inference' to classify the image."; | |
| ResultsTextBox.Text = _session != null | |
| ? "Image selected. Click 'Run Inference' to classify the image." | |
| : "Image selected. Load or reload the model first to enable inference."; |
| 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; | ||
| EpCombo.IsEnabled = !busy; | ||
| DeviceCombo.IsEnabled = !busy; | ||
| AllowProviderDownloadCheckBox.IsEnabled = !busy; | ||
| } |
There was a problem hiding this comment.
SetBusy disables EP/device selection and provider download, but leaves the Perf Mode radio buttons enabled. Since Perf Mode is read during model load and affects session creation, it should also be disabled while busy to avoid users changing configuration mid-load/inference and getting inconsistent behavior.
| var session = _session; | ||
| using var results = await Task.Run(() => InferenceEngine.RunInference(session, inputTensor)); | ||
| var resultTensor = InferenceEngine.ExtractResults(_session, results); | ||
|
|
There was a problem hiding this comment.
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.
…, use captured session Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using var results = InferenceEngine.RunInference(_session, inputTensor); | ||
| var resultTensor = InferenceEngine.ExtractResults(_session, results); | ||
| var session = _session; | ||
| using var results = await Task.Run(() => InferenceEngine.RunInference(session, inputTensor)); |
There was a problem hiding this comment.
Running inference on a background thread introduces a new lifecycle/concurrency edge case: the window can be closed while the Task.Run inference is still executing, and MainWindow.Dispose() will dispose _session concurrently with session.Run. This can lead to ObjectDisposedException or native failures. Consider preventing window close while busy, or tracking the in-flight inference task and awaiting it (or otherwise coordinating) before disposing the session/environment.
| using var results = await Task.Run(() => InferenceEngine.RunInference(session, inputTensor)); | |
| using var results = InferenceEngine.RunInference(session, inputTensor); |
| 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; | ||
| EpCombo.IsEnabled = !busy; | ||
| DeviceCombo.IsEnabled = !busy; |
There was a problem hiding this comment.
SetBusy(bool) unconditionally sets DeviceCombo.IsEnabled and EpCombo.IsEnabled to !busy, which can override the intended enabled/disabled state managed by PopulateDeviceCombo (e.g., when only one device type exists or when there are no devices for the selected EP). After an inference/model load completes, this can leave DeviceCombo enabled with no valid selections and cause LoadModelAndLabelsAsync to prompt for a device type that cannot be selected. Consider restoring each control to its logical enabled state (e.g., recompute from current EP/devices, or disable a parent container while busy instead of forcing individual IsEnabled values).
| 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; | |
| EpCombo.IsEnabled = !busy; | |
| DeviceCombo.IsEnabled = !busy; | |
| private bool _epComboEnabledBeforeBusy; | |
| private bool _deviceComboEnabledBeforeBusy; | |
| private void SetBusy(bool busy) | |
| { | |
| if (busy) | |
| { | |
| _epComboEnabledBeforeBusy = EpCombo.IsEnabled; | |
| _deviceComboEnabledBeforeBusy = DeviceCombo.IsEnabled; | |
| } | |
| BusyIndicator.Visibility = busy ? Visibility.Visible : Visibility.Collapsed; | |
| RunInferenceButton.IsEnabled = !busy && _session != null && !string.IsNullOrEmpty(_selectedImagePath); | |
| ReloadSessionButton.IsEnabled = !busy; | |
| SelectImageButton.IsEnabled = !busy; | |
| EpCombo.IsEnabled = busy ? false : _epComboEnabledBeforeBusy; | |
| DeviceCombo.IsEnabled = busy ? false : _deviceComboEnabledBeforeBusy; |
…t close during inference Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| SetBusy(true); |
There was a problem hiding this comment.
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; |
| ResultsTextBox.Text = "Running inference..."; | ||
| Dispatcher.Invoke(() => { }, System.Windows.Threading.DispatcherPriority.Render); | ||
|
|
||
| var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); |
There was a problem hiding this comment.
ImageProcessor.LoadImageFileAsync returns a VideoFrame (disposable). Unlike the WinUI/WinForms versions of this sample, this handler doesn’t dispose the videoFrame, which can leak native resources/file handles across repeated inferences. Use a using/Dispose pattern for the returned VideoFrame (and ensure any downstream objects that hold it are disposed as needed).
| var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); | |
| using var videoFrame = await ImageProcessor.LoadImageFileAsync(_selectedImagePath); |
…n from IsEnabled Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
Samples/WindowsML/cs-wpf/MainWindow.xaml.cs:233
- RunInferenceButton_Click 的前置校验同时覆盖了“未选图片”和“模型未加载(_session==null)”两种情况,但提示文本固定为“Please select an image first.”。在本 PR 中你已经在选图后根据 _session 是否为 null 给出了不同提示,这里建议也区分两种情况(例如分别提示先加载模型 / 先选择图片),避免用户在模型未加载时被误导。
if (string.IsNullOrEmpty(_selectedImagePath) || _session == null)
{
ResultsTextBox.Text = "Please select an image first.";
return;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
The WPF WindowsML sample's
RunInferencecall was blocking the UI thread, risking freezes on larger models. This PR fixes the threading issue and adds a visual busy indicator.Changes
1. Move inference to background thread (
Task.Run)InferenceEngine.RunInferenceis synchronous (callssession.Run). Even though the calling method isasync, priorawaitcalls resume on the UI thread via WPF'sSynchronizationContext, soRunInferencestill blocks the UI thread.await Task.Run(...)to offload to a thread pool thread.2. Add indeterminate ProgressBar busy indicator
ProgressBarwithIsIndeterminate=Truethat appears during inference and model loading.SetBusy(bool)helper that toggles the progress bar and disables interactive buttons (Run Inference, Select Image, Load/Reload Model) to prevent double-clicks.RunInferenceButton_ClickandReloadSessionButton_Clickwithtry/finally.Dispatcher.Invokerender-flush workaround (no longer needed).Testing
dotnet build(0 errors, 0 warnings)dotnet runand verified the window launches correctlyFixes ADO #61791035