Add conversational awareness volume control and fix volume state management#7
Conversation
…eness volume slider Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
… bugs Co-authored-by: YimingZhanshen <76594627+YimingZhanshen@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds user-configurable volume control for the conversational awareness feature and fixes several volume state management bugs. The changes address issues where volume would incorrectly restore to 100% instead of the pre-speaking level, and adds a workaround for unreliable restoration events in transparency mode.
Changes:
- Added a configurable slider (10-100%, default 40%) for conversational awareness volume reduction
- Fixed volume state management using session-based tracking to prevent incorrect restoration
- Disabled conversational awareness in transparency mode due to firmware limitations
- Updated version to 1.2.0 and corrected MagicAAP driver attribution links
- Fixed x64 video widget rendering issues by adjusting show/hide logic
- Added Chinese translations for the new volume slider
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| vcpkg.json | Version bump to 1.2.0 |
| CMakeLists.txt | Version bump to 1.2.0 |
| README.md, README-CN.md, README-TW.md | Updated MagicAAP driver link from GitHub to official site |
| Source/Gui/SettingsWindow.ui | Added volume slider UI controls and updated row numbering |
| Source/Gui/SettingsWindow.h | Added slider value change handler declaration |
| Source/Gui/SettingsWindow.cpp | Implemented slider UI binding, updated credits, fixed version link |
| Source/Core/Settings.h | Added conversational_awareness_volume_percent field |
| Source/Core/Settings.cpp | Implemented volume percent setting handler |
| Source/Core/GlobalMedia_win.h | Added _inReductionSession flag and ClearVolumeReductionState method |
| Source/Core/GlobalMedia_win.cpp | Implemented session-based volume state tracking with persistent saved volume |
| Source/Core/GlobalMedia.h | Exposed ClearVolumeReductionState in public API |
| Source/Core/AirPods.h | Added volume percent and noise control mode tracking members |
| Source/Core/AirPods.cpp | Implemented configurable volume reduction, transparency mode check, intermediate level handling |
| Source/Gui/MainWindow.cpp | Fixed x64 video widget transparency issue by adjusting visibility logic |
| Source/Resource/Translation/apd_zh_CN.ts | Added Simplified Chinese translation for volume slider |
| Source/Resource/Translation/apd_zh_TW.ts | Added Traditional Chinese translation for volume slider |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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); |
There was a problem hiding this comment.
The saved volume state management has an edge case issue. When volume restoration occurs (line 598-604), _savedVolume is not cleared. This is intentional to handle duplicate restoration calls from AAP firmware. However, if the system volume happens to be at 0% when a reduction session starts, _savedVolume will be set to 0, and then hasVolumeReduction (line 562: _savedVolume > 0) will be false. This means:
- Future restoration calls (SetVolume(100)) won't restore anything because hasVolumeReduction is false
- The session flag will still be set, preventing new sessions from starting
This is an extremely rare edge case (volume at 0%), but it could cause the feature to get stuck. Consider using _inReductionSession as the primary indicator instead of _savedVolume > 0, or handle the 0% case explicitly.
|
|
||
| // 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
Setting the video output to nullptr and then immediately back to _videoWidget seems unnecessary. The comment suggests this is needed for x64 builds, but there's no clear explanation of why this double-set is required. This pattern is unusual and could be fragile.
If this is truly necessary for x64 rendering, consider adding a more detailed comment explaining the specific Qt/Windows x64 bug being worked around, or investigate whether there's a cleaner solution.
| // 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); | |
| } |
| // Stop the player but keep widget visible to avoid x64 rendering issues | ||
| // The widget will be black when stopped, but this is better than becoming | ||
| // transparent/hollow on x64 builds |
There was a problem hiding this comment.
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).
|
|
||
| // 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) { |
There was a problem hiding this comment.
The _currentNoiseControlMode member is accessed in OnSpeakingLevelChanged without mutex protection, but is modified in OnNoiseControlModeNotification with the mutex locked. Both methods can be called from different threads (AAP callbacks), creating a potential race condition.
The OnSpeakingLevelChanged method reads _currentNoiseControlMode at lines 994-995 without holding _mutex, while OnNoiseControlModeNotification at line 922 sets it while holding _mutex. This violates thread safety and could lead to undefined behavior or inconsistent reads.
Ensure _mutex is locked when accessing _currentNoiseControlMode in OnSpeakingLevelChanged.
| // 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) { |
| LOG(Info, "User started speaking - reducing media volume to {}%", _conversationalAwarenessVolumePercent); | ||
| Core::GlobalMedia::SetVolume(_conversationalAwarenessVolumePercent); | ||
| break; |
There was a problem hiding this comment.
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; | |
| } |
| { | ||
| if (!_conversationalAwarenessEnabled) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
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.
| </message> | ||
| <message> | ||
| <source>Speaking volume reduction:</source> | ||
| <translation>說話時音量降低至:</translation> |
There was a problem hiding this comment.
The Traditional Chinese translation "說話時音量降低至:" could be clearer. It translates to "When speaking, volume reduces to:" which doesn't explicitly clarify that this is a percentage of the pre-speaking volume, not an absolute value.
Consider a more explicit translation like "說話時音量百分比:" (Speaking volume percentage:) or "說話時保持音量:" (Volume to maintain when speaking:) to better match the English description "Volume percentage when speaking (based on your pre-speaking volume)."
| <translation>說話時音量降低至:</translation> | |
| <translation>說話時音量百分比:</translation> |
| default: | ||
| // Intermediate levels - could implement gradual volume adjustment | ||
| // Intermediate levels (0x04-0x07) - restore volume to be safe | ||
| // This ensures volume is restored even if final event is missed | ||
| if (static_cast<uint8_t>(level) >= 0x04 && static_cast<uint8_t>(level) <= 0x07) { | ||
| LOG(Info, "Intermediate speaking level detected - restoring media volume"); | ||
| Core::GlobalMedia::SetVolume(kFullVolumePercent); | ||
| } |
There was a problem hiding this comment.
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.
| void Controller::ClearVolumeReductionState() | ||
| { | ||
| std::lock_guard<std::mutex> lock{_mutex}; | ||
| _savedVolume = 0; | ||
| _inReductionSession = false; | ||
| LOG(Trace, "ClearVolumeReductionState: Cleared saved volume state"); | ||
| } |
There was a problem hiding this comment.
The ClearVolumeReductionState() function is defined and exposed in the public API but is never called anywhere in the codebase. This appears to be dead code that should either be used (e.g., when conversational awareness is disabled, when the device disconnects, or when noise control mode changes) or removed.
| </message> | ||
| <message> | ||
| <source>Speaking volume reduction:</source> | ||
| <translation>说话时音量降低至:</translation> |
There was a problem hiding this comment.
The Chinese translation "说话时音量降低至:" (Simplified) and "說話時音量降低至:" (Traditional) could be clearer. They translate to "When speaking, volume reduces to:" which doesn't explicitly clarify that this is a percentage of the pre-speaking volume, not an absolute value.
Consider a more explicit translation like "说话时音量百分比:" (Speaking volume percentage:) or "说话时保持音量:" (Volume to maintain when speaking:) to better match the English description "Volume percentage when speaking (based on your pre-speaking volume)."
| <translation>说话时音量降低至:</translation> | |
| <translation>说话时音量百分比:</translation> |
Conversational awareness lacked user control over volume reduction and had persistent state bugs causing incorrect restoration. AAP firmware in transparency mode unreliably sends restoration events, causing volume to remain reduced.
Volume Control
Volume State Management
Volume was saved once at program start and never updated, preventing user adjustments. Multiple AAP restoration events caused volume to jump to 100%.
Session-based state tracking:
Persistent saved volume:
Transparency Mode
Disabled conversational awareness in transparency mode due to firmware limitation where restoration events are unreliable:
Additional Changes
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.