-
Notifications
You must be signed in to change notification settings - Fork 1
Add conversational awareness volume control and fix volume state management #7
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
Changes from all commits
700527d
8e87d94
fd5bcf6
6b4e67f
6530f86
8e6fce5
89a00f1
30d9c96
ed3085b
87a2583
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 | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -865,6 +865,14 @@ void Manager::OnConversationalAwarenessChanged(bool enable) | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| void Manager::OnConversationalAwarenessVolumePercentChanged(uint8_t percent) | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| std::lock_guard<std::mutex> lock{_mutex}; | ||||||||||||||||||||||||||||||||||||
| // Clamp the value to valid range (10-100) matching UI slider constraints | ||||||||||||||||||||||||||||||||||||
| _conversationalAwarenessVolumePercent = std::clamp(percent, uint8_t{10}, uint8_t{100}); | ||||||||||||||||||||||||||||||||||||
| LOG(Info, "Conversational awareness volume percent changed to {}%", _conversationalAwarenessVolumePercent); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| void Manager::OnPersonalizedVolumeChanged(bool enable) | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| std::lock_guard<std::mutex> lock{_mutex}; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -910,6 +918,9 @@ void Manager::OnNoiseControlModeNotification(AAP::NoiseControlMode mode) | |||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| LOG(Info, "Noise control mode changed to: {}", Helper::ToString(mode).toStdString()); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Track the current noise control mode | ||||||||||||||||||||||||||||||||||||
| _currentNoiseControlMode = mode; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Update the cached state in the state manager if we have a current state | ||||||||||||||||||||||||||||||||||||
| auto state = _stateMgr.GetCurrentState(); | ||||||||||||||||||||||||||||||||||||
| if (state.has_value()) { | ||||||||||||||||||||||||||||||||||||
|
|
@@ -968,20 +979,29 @@ void Manager::OnHeadTrackingData(AAP::HeadTrackingData data) | |||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Volume levels for conversational awareness | ||||||||||||||||||||||||||||||||||||
| constexpr int kConversationalAwarenessVolumePercent = 40; // Volume when user is speaking | ||||||||||||||||||||||||||||||||||||
| constexpr int kFullVolumePercent = 100; // Normal volume when not speaking | ||||||||||||||||||||||||||||||||||||
| // kFullVolumePercent (100) signals to GlobalMedia::SetVolume to restore the saved pre-speaking volume | ||||||||||||||||||||||||||||||||||||
| // The actual restoration logic is in GlobalMedia_win.cpp which restores to the saved volume, not literally 100% | ||||||||||||||||||||||||||||||||||||
| constexpr int kFullVolumePercent = 100; // Signal value to restore to original volume | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| void Manager::OnSpeakingLevelChanged(AAP::SpeakingLevel level) | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| if (!_conversationalAwarenessEnabled) { | ||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| // Disable conversational awareness in transparency mode to avoid volume restoration bugs | ||||||||||||||||||||||||||||||||||||
| // The AAP firmware in transparency mode doesn't reliably send restoration events | ||||||||||||||||||||||||||||||||||||
| if (_currentNoiseControlMode.has_value() && | ||||||||||||||||||||||||||||||||||||
| _currentNoiseControlMode.value() == AAP::NoiseControlMode::Transparency) { | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
991
to
+995
|
||||||||||||||||||||||||||||||||||||
| // Disable conversational awareness in transparency mode to avoid volume restoration bugs | |
| // The AAP firmware in transparency mode doesn't reliably send restoration events | |
| if (_currentNoiseControlMode.has_value() && | |
| _currentNoiseControlMode.value() == AAP::NoiseControlMode::Transparency) { | |
| // Take a thread-safe snapshot of the current noise control mode | |
| std::optional<AAP::NoiseControlMode> currentNoiseControlModeCopy; | |
| { | |
| std::lock_guard<std::mutex> lock(_mutex); | |
| currentNoiseControlModeCopy = _currentNoiseControlMode; | |
| } | |
| // Disable conversational awareness in transparency mode to avoid volume restoration bugs | |
| // The AAP firmware in transparency mode doesn't reliably send restoration events | |
| if (currentNoiseControlModeCopy.has_value() && | |
| currentNoiseControlModeCopy.value() == AAP::NoiseControlMode::Transparency) { |
Copilot
AI
Feb 2, 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.
The _conversationalAwarenessVolumePercent member is accessed at line 1003-1004 without mutex protection, but is modified in OnConversationalAwarenessVolumePercentChanged with the mutex locked. Both methods can be called from different threads, creating a race condition.
OnSpeakingLevelChanged reads _conversationalAwarenessVolumePercent without holding _mutex, while OnConversationalAwarenessVolumePercentChanged at line 870 modifies it while holding _mutex. This violates thread safety.
Ensure _mutex is locked when accessing _conversationalAwarenessVolumePercent in OnSpeakingLevelChanged.
| LOG(Info, "User started speaking - reducing media volume to {}%", _conversationalAwarenessVolumePercent); | |
| Core::GlobalMedia::SetVolume(_conversationalAwarenessVolumePercent); | |
| break; | |
| { | |
| int volumePercent; | |
| { | |
| std::lock_guard lock(_mutex); | |
| volumePercent = _conversationalAwarenessVolumePercent; | |
| } | |
| LOG(Info, "User started speaking - reducing media volume to {}%", volumePercent); | |
| Core::GlobalMedia::SetVolume(volumePercent); | |
| break; | |
| } |
Copilot
AI
Feb 2, 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.
The intermediate speaking level restoration (0x04-0x07) could trigger restoration even when the volume was never reduced. If conversational awareness was disabled or the feature just started, receiving an intermediate level event (0x04-0x07) would call SetVolume(100) without any prior reduction having occurred.
The check for _conversationalAwarenessEnabled at the start of OnSpeakingLevelChanged protects against this when the feature is disabled, but if the feature is enabled and an intermediate level is received before any actual reduction events, it could still trigger an unwanted restoration. Consider checking if a reduction session is active before restoring on intermediate levels.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -561,9 +561,10 @@ void Controller::SetVolume(int percent) | |
| bool isReducing = percent < 100; | ||
| bool hasVolumeReduction = _savedVolume > 0; | ||
|
|
||
| // Save current volume before reducing (if this is the first reduction) | ||
| if (isReducing && !hasVolumeReduction) { | ||
| // Get actual current volume | ||
| // Save current volume before reducing | ||
| // ALWAYS re-read the actual volume to ensure we capture user's manual changes | ||
| if (isReducing && !_inReductionSession) { | ||
| // Get actual current volume from the system | ||
| int currentVolume = 100; | ||
| try { | ||
| OS::Windows::Com::UniquePtr<IMMDeviceEnumerator> deviceEnumerator; | ||
|
|
@@ -588,18 +589,23 @@ void Controller::SetVolume(int percent) | |
| } | ||
| } catch (...) {} | ||
| _savedVolume = currentVolume; | ||
| _inReductionSession = true; | ||
| LOG(Trace, "SetVolume: Saved current volume {}% before reduction, starting session", _savedVolume); | ||
| } | ||
|
|
||
| // Calculate target volume | ||
| int targetPercent; | ||
| if (percent == 100 && hasVolumeReduction) { | ||
| // Restoring - use saved volume | ||
| // Keep _savedVolume to handle duplicate restoration calls | ||
| // It will be cleared when the next reduction cycle starts | ||
| targetPercent = _savedVolume; | ||
| // Clear saved volume after restoring | ||
| _savedVolume = 0; | ||
| _inReductionSession = false; | ||
| LOG(Trace, "SetVolume: Restoring to saved volume {}%, ending session", targetPercent); | ||
|
Comment on lines
598
to
+604
|
||
| } else if (isReducing && _savedVolume > 0) { | ||
| // Reducing - calculate relative to saved volume | ||
| targetPercent = (percent * _savedVolume) / 100; | ||
| LOG(Trace, "SetVolume: Reducing to {}% ({}% of saved {}%)", targetPercent, percent, _savedVolume); | ||
| } else { | ||
| // No reduction state - just set the percent | ||
| targetPercent = percent; | ||
|
|
@@ -687,4 +693,13 @@ int Controller::GetVolume() const | |
| return 100; | ||
| } | ||
| } | ||
|
|
||
| void Controller::ClearVolumeReductionState() | ||
| { | ||
| std::lock_guard<std::mutex> lock{_mutex}; | ||
| _savedVolume = 0; | ||
| _inReductionSession = false; | ||
| LOG(Trace, "ClearVolumeReductionState: Cleared saved volume state"); | ||
| } | ||
|
Comment on lines
+697
to
+703
|
||
|
|
||
| } // namespace Core::GlobalMedia | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -472,22 +472,26 @@ void MainWindow::PlayAnimation() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _isAnimationPlaying = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Stop player and reset position for clean replay | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Qt's QMediaPlayer handles these synchronously in the event loop | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _mediaPlayer->stop(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _mediaPlayer->setPosition(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _videoWidget->show(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Reinitialize video widget rendering surface after hide/show cycle | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This is called only on window show (not during playback loop), so performance impact is minimal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _mediaPlayer->setVideoOutput(_videoWidget); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure video widget is visible - critical for x64 builds | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The widget must be visible and have its video output set before playing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!_videoWidget->isVisible()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _videoWidget->show(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On x64, we need to ensure the video output is properly connected after showing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _mediaPlayer->setVideoOutput(static_cast<QVideoWidget*>(nullptr)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| _mediaPlayer->setVideoOutput(_videoWidget); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+477
to
+486
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure video widget is visible - critical for x64 builds | |
| // The widget must be visible and have its video output set before playing | |
| if (!_videoWidget->isVisible()) { | |
| _videoWidget->show(); | |
| // On x64, we need to ensure the video output is properly connected after showing | |
| _mediaPlayer->setVideoOutput(static_cast<QVideoWidget*>(nullptr)); | |
| _mediaPlayer->setVideoOutput(_videoWidget); | |
| } | |
| // Ensure video widget is visible - critical for x64 builds. | |
| // NOTE: On some Windows x64 configurations (Qt + DirectShow backend), if the | |
| // QVideoWidget is shown *after* a full stop(), the first frame is never | |
| // painted and the surface stays black/transparent unless the video output | |
| // is temporarily cleared and rebound. Explicitly setting the output to | |
| // nullptr forces the underlying video surface to be torn down, and | |
| // immediately re‑attaching _videoWidget causes Qt to recreate and properly | |
| // initialize the rendering pipeline for the newly shown widget. | |
| // | |
| // This sequence has been validated to fix the x64 rendering issue and | |
| // must not be "simplified" by removing either call unless the underlying | |
| // Qt/Windows bug is confirmed to be resolved and re‑tested on affected | |
| // platforms. | |
| if (!_videoWidget->isVisible()) { | |
| _videoWidget->show(); | |
| _mediaPlayer->setVideoOutput(static_cast<QVideoWidget*>(nullptr)); | |
| _mediaPlayer->setVideoOutput(_videoWidget); | |
| } |
Copilot
AI
Feb 2, 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.
Keeping the widget visible when stopped means users will see a black rectangle instead of the widget being hidden. The comment acknowledges this ("The widget will be black when stopped") but doesn't explain why this is preferable to the transparency issue.
This could be confusing to users who see a black box instead of the expected animation or nothing at all. Consider whether there's a better solution that avoids both the transparency bug and the black rectangle, or add UI polish to make the stopped state less jarring (e.g., show a static image).
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.
The _conversationalAwarenessEnabled member is accessed at line 988 without mutex protection, but is modified in OnConversationalAwarenessChanged with the mutex locked. Both methods can be called from different threads, creating a race condition.
OnSpeakingLevelChanged reads _conversationalAwarenessEnabled without holding _mutex, while OnConversationalAwarenessChanged at line 860 modifies it while holding _mutex. This violates thread safety.
Ensure _mutex is locked when accessing _conversationalAwarenessEnabled in OnSpeakingLevelChanged.