[Linux] Fix ear detection pause/resume with playerctl fallback #424
Conversation
- Remove isActiveOutputDeviceAirPods() check from pause/resume logic as it fails on some systems where PulseAudio can't detect the Bluetooth card properly - Add playerctl as fallback when DBus MPRIS doesn't find playing media players. This fixes compatibility with browsers and players that don't register properly with DBus on certain systems - Resume logic now correctly uses playerctl if that's how media was paused Fixes ear detection not pausing media on aarch64 Ubuntu systems.
📝 WalkthroughWalkthroughModified pause and resume logic in the media controller to remove AirPods-specific device checks and implement playerctl-based fallback paths. The play() function now attempts playerctl-based resume first if paused by app services, while pause() adds a playerctl fallback when DBus-based pausing fails. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @linux/media/mediacontroller.cpp:
- Around line 310-325: The current early return after
QProcess::execute("playerctl", ...) leaves pausedByAppServices containing
"playerctl" on failure and causes repeated failed attempts; update the playerctl
branch in resume logic (the block that checks
pausedByAppServices.contains("playerctl") and calls QProcess::execute) to clear
or remove the "playerctl" entry from pausedByAppServices on both success and
failure before returning, and on failure additionally attempt the DBus fallback
path (same code used by the non-playerctl path) or log a clear error and
continue to other resume mechanisms; ensure you reference pausedByAppServices,
QProcess::execute, LOG_WARN/LOG_INFO and the surrounding resume method so the
state is always cleared on failure to avoid repeated retries.
🧹 Nitpick comments (1)
linux/media/mediacontroller.cpp (1)
421-432: Consider WARN level when playerctl fallback fails.When both DBus and playerctl fail to pause media, logging at DEBUG level may make issues hard to diagnose. Since this is the last-resort fallback, a WARN would be more appropriate:
Suggested change
else { - LOG_DEBUG("playerctl fallback returned: " << result); + LOG_WARN("playerctl fallback failed with code: " << result); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linux/media/mediacontroller.cpp
🔇 Additional comments (2)
linux/media/mediacontroller.cpp (2)
52-58: Reasonable fix for PulseAudio compatibility issues.Removing the
isActiveOutputDeviceAirPods()check broadens the pause trigger to work regardless of whether PulseAudio can detect the Bluetooth card. This is a sensible trade-off for systems where the detection fails.Note: This means ear detection will pause media even when audio output is routed elsewhere (e.g., speakers). If this becomes an issue, consider logging a warning when the active device check fails rather than skipping it entirely.
67-70: LGTM!Consistent with the pause logic change. The added logging will help with debugging resume events.
| // If we paused via playerctl fallback, resume via playerctl | ||
| if (pausedByAppServices.contains("playerctl")) | ||
| { | ||
| LOG_INFO("Resuming media via playerctl"); | ||
| int result = QProcess::execute("playerctl", QStringList() << "play"); | ||
| if (result == 0) | ||
| { | ||
| LOG_INFO("Resumed media via playerctl"); | ||
| pausedByAppServices.clear(); | ||
| } | ||
| else | ||
| { | ||
| LOG_WARN("playerctl play returned: " << result); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Failure path leaves stale state, causing repeated failures.
If playerctl play fails (e.g., playerctl not installed, no player available), pausedByAppServices is not cleared and the function returns early. Subsequent resume attempts will keep trying playerctl and failing indefinitely.
Consider clearing the state on failure or falling back to DBus:
Suggested fix
int result = QProcess::execute("playerctl", QStringList() << "play");
if (result == 0)
{
LOG_INFO("Resumed media via playerctl");
- pausedByAppServices.clear();
}
else
{
LOG_WARN("playerctl play returned: " << result);
}
+ pausedByAppServices.clear(); // Clear regardless to avoid infinite retry loop
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If we paused via playerctl fallback, resume via playerctl | |
| if (pausedByAppServices.contains("playerctl")) | |
| { | |
| LOG_INFO("Resuming media via playerctl"); | |
| int result = QProcess::execute("playerctl", QStringList() << "play"); | |
| if (result == 0) | |
| { | |
| LOG_INFO("Resumed media via playerctl"); | |
| pausedByAppServices.clear(); | |
| } | |
| else | |
| { | |
| LOG_WARN("playerctl play returned: " << result); | |
| } | |
| return; | |
| } | |
| // If we paused via playerctl fallback, resume via playerctl | |
| if (pausedByAppServices.contains("playerctl")) | |
| { | |
| LOG_INFO("Resuming media via playerctl"); | |
| int result = QProcess::execute("playerctl", QStringList() << "play"); | |
| if (result == 0) | |
| { | |
| LOG_INFO("Resumed media via playerctl"); | |
| } | |
| else | |
| { | |
| LOG_WARN("playerctl play returned: " << result); | |
| } | |
| pausedByAppServices.clear(); // Clear regardless to avoid infinite retry loop | |
| return; | |
| } |
🤖 Prompt for AI Agents
In @linux/media/mediacontroller.cpp around lines 310 - 325, The current early
return after QProcess::execute("playerctl", ...) leaves pausedByAppServices
containing "playerctl" on failure and causes repeated failed attempts; update
the playerctl branch in resume logic (the block that checks
pausedByAppServices.contains("playerctl") and calls QProcess::execute) to clear
or remove the "playerctl" entry from pausedByAppServices on both success and
failure before returning, and on failure additionally attempt the DBus fallback
path (same code used by the non-playerctl path) or log a clear error and
continue to other resume mechanisms; ensure you reference pausedByAppServices,
QProcess::execute, LOG_WARN/LOG_INFO and the surrounding resume method so the
state is always cleared on failure to avoid repeated retries.
1 participant
Remove isActiveOutputDeviceAirPods() check from pause/resume logic as it fails on some systems where PulseAudio can't detect the Bluetooth card properly
Add playerctl as fallback when DBus MPRIS doesn't find playing media players. This fixes compatibility with browsers and players that don't register properly with DBus on certain systems
Resume logic now correctly uses playerctl if that's how media was paused
Fixes ear detection not pausing media on aarch64 Ubuntu systems.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.