linux: fix connected device detection failure when using wireplumber 0.5.13#417
linux: fix connected device detection failure when using wireplumber 0.5.13#417jamescarter2001 wants to merge 2 commits intokavishdevar:mainfrom
Conversation
📝 WalkthroughWalkthroughMAC address handling was changed to pass raw MACs (with colons) to MediaController; device matching now uses exact MAC equality. PulseAudioController gained APIs to fetch a sink's device.string (MAC) and the default sink's MAC to support these comparisons. Changes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
jamescarter2001
force-pushed
the
wireplumber-connected-device-fix
branch
from
5181b65 to
e4048a2
Compare
![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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
linux/main.cpplinux/media/mediacontroller.cpplinux/media/pulseaudiocontroller.cpplinux/media/pulseaudiocontroller.h
🧰 Additional context used
🧬 Code graph analysis (1)
linux/main.cpp (1)
linux/deviceinfo.hpp (3)
address(138-145)address(138-138)address(219-219)
🔇 Additional comments (9)
linux/media/mediacontroller.cpp (1)
97-101: LGTM! More robust device detection using exact MAC comparison.The switch from substring matching on sink names to exact MAC address comparison is a solid fix for the wireplumber 0.5.13 regression. This approach is less brittle and won't break if sink name formatting changes again.
linux/media/pulseaudiocontroller.h (2)
18-18: LGTM! Well-designed public API addition.The new
getDefaultSinkMacAddress()method provides a clean interface for retrieving the MAC address of the default audio sink.
36-36: Good encapsulation with private helper method.Making
getMacAddressBySinkName()private is appropriate since it's an implementation detail.linux/main.cpp (4)
106-111: LGTM! Standardized MAC address format on startup.Passing the raw MAC address directly (with colons) ensures consistency with how PulseAudio's
device.stringproperty is formatted.
426-437: LGTM! Consistent MAC handling after system wake.Using the raw Bluetooth address format aligns with the new MAC-based device matching approach.
492-500: LGTM! Standardized MAC format for BlueZ connections.The raw address format ensures proper matching with PulseAudio device properties.
738-742: LGTM! Consistent MAC handling in metadata parsing flow.Using the raw Bluetooth address maintains consistency across all device connection paths.
linux/media/pulseaudiocontroller.cpp (2)
114-116: LGTM! Clean delegation pattern.The implementation correctly delegates to the helper method for retrieving the default sink's MAC address.
246-288: LGTM! More robust MAC-based card matching.The refactored
getCardNameForDevice()now uses exact MAC address comparison viadevice.stringinstead of name-based pattern matching. This is more reliable and fixes the wireplumber 0.5.13 regression where name formatting changed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
linux/media/pulseaudiocontroller.cpp (1)
267-274: Handle potential null pointer frompa_proplist_gets.
pa_proplist_getsreturnsNULLif the property doesn't exist. Directly assigning toQStringwithout a null check can cause undefined behavior or create an invalidQString. This differs from the safer pattern used ingetMacAddressBySinkName(lines 137-146) which checksif (addr)before use.🔎 Proposed fix
if (info) { const QString name = QString::fromUtf8(info->name); - const QString macAddress = pa_proplist_gets(info->proplist, "device.string"); + const char *addr = pa_proplist_gets(info->proplist, "device.string"); + const QString macAddress = addr ? QString::fromUtf8(addr) : QString(); if (d->targetMac == macAddress)
♻️ Duplicate comments (1)
linux/media/pulseaudiocontroller.cpp (1)
150-150: UsetoUtf8().constData()for consistency.This was flagged in a previous review: line 150 uses
toLocal8Bit()while all other equivalent calls in this file usetoUtf8().constData()(lines 190, 215, 236-237, 327).🔎 Proposed fix
- pa_operation *op = pa_context_get_sink_info_by_name(m_context, sinkName.toLocal8Bit(), callback, &data); + pa_operation *op = pa_context_get_sink_info_by_name(m_context, sinkName.toUtf8().constData(), callback, &data);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
linux/main.cpplinux/media/mediacontroller.cpplinux/media/pulseaudiocontroller.cpplinux/media/pulseaudiocontroller.h
🚧 Files skipped from review as they are similar to previous changes (2)
- linux/media/pulseaudiocontroller.h
- linux/main.cpp
🔇 Additional comments (2)
linux/media/pulseaudiocontroller.cpp (1)
114-116: LGTM!Clean delegation pattern. The explicit
this->is optional but harmless.linux/media/mediacontroller.cpp (1)
97-101: No action required—MAC address format handling is correct.The change correctly addresses the wireplumber 0.5.13 regression. The comparison uses exact equality (
==) betweendefaultSinkMacAddressfrom PulseAudio'sdevice.stringproperty andconnectedDeviceMacAddressfromQBluetoothAddress::toString(). Both return standard colon-separated MAC addresses (e.g.,AA:BB:CC:DD:EE:FF), so format consistency is guaranteed. The fix specifically removed the.replace(":", "_")transformation that was causing mismatches with the previous substring-based logic.
jamescarter2001
changed the title
jamescarter2001
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
linux/media/pulseaudiocontroller.cpp (2)
114-116: Optional: Remove explicitthis->for consistency.The explicit
this->qualifier is unnecessary in C++ and inconsistent with the rest of the codebase (e.g., lines 102, 106, 109 don't use it). Consider simplifying toreturn getMacAddressBySinkName(getDefaultSink());🔎 Proposed refactor
QString PulseAudioController::getDefaultSinkMacAddress() { - return this->getMacAddressBySinkName(this->getDefaultSink()); + return getMacAddressBySinkName(getDefaultSink()); }
267-270: Add defensive null check and document device.string assumption.While
QStringsafely handles NULL frompa_proplist_gets, making the null check explicit improves clarity. Additionally, as noted in past reviews, the assumption thatdevice.stringcontains a MAC address may not hold across all PulseAudio/WirePlumber versions—consider documenting this dependency.🔎 Proposed improvements
if (info) { const QString name = QString::fromUtf8(info->name); - const QString macAddress = pa_proplist_gets(info->proplist, "device.string"); + // device.string typically contains the Bluetooth MAC address for BT devices + // but availability may vary across PulseAudio/WirePlumber versions + const char *deviceStr = pa_proplist_gets(info->proplist, "device.string"); + const QString macAddress = deviceStr ? QString::fromUtf8(deviceStr) : QString(); if (d->targetMac == macAddress) { d->cardName = name; pa_threaded_mainloop_signal(d->mainloop, 0); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
linux/media/pulseaudiocontroller.cpp
🔇 Additional comments (1)
linux/media/pulseaudiocontroller.cpp (1)
150-150: Encoding consistency restored.The previous encoding inconsistency flagged in past reviews has been correctly addressed. Line 150 now uses
toUtf8().constData(), matching the convention used throughout the file (lines 190, 215, 236-237, 327).
| auto callback = [](pa_context *c, const pa_sink_info *info, int eol, void *userdata) | ||
| { | ||
| CallbackData *d = static_cast<CallbackData*>(userdata); | ||
| if (eol > 0) | ||
| { | ||
| pa_threaded_mainloop_signal(d->mainloop, 0); | ||
| return; | ||
| } | ||
|
|
||
| const char *addr = pa_proplist_gets( | ||
| info->proplist, | ||
| "device.string" | ||
| ); | ||
|
|
||
| if (addr) | ||
| { | ||
| d->sinkMacAddress = QString::fromUtf8(addr); | ||
| pa_threaded_mainloop_signal(d->mainloop, 0); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical: Mainloop signal missing when device.string property is absent.
If pa_proplist_gets returns NULL (property doesn't exist), the callback never signals the mainloop, causing waitForOperation to hang indefinitely. The mainloop must be signaled in all code paths.
🔎 Proposed fix
auto callback = [](pa_context *c, const pa_sink_info *info, int eol, void *userdata)
{
CallbackData *d = static_cast<CallbackData*>(userdata);
if (eol > 0)
{
pa_threaded_mainloop_signal(d->mainloop, 0);
return;
}
const char *addr = pa_proplist_gets(
info->proplist,
"device.string"
);
if (addr)
{
d->sinkMacAddress = QString::fromUtf8(addr);
- pa_threaded_mainloop_signal(d->mainloop, 0);
}
+ // Always signal, even if device.string is not present
+ pa_threaded_mainloop_signal(d->mainloop, 0);
};📝 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.
| auto callback = [](pa_context *c, const pa_sink_info *info, int eol, void *userdata) | |
| { | |
| CallbackData *d = static_cast<CallbackData*>(userdata); | |
| if (eol > 0) | |
| { | |
| pa_threaded_mainloop_signal(d->mainloop, 0); | |
| return; | |
| } | |
| const char *addr = pa_proplist_gets( | |
| info->proplist, | |
| "device.string" | |
| ); | |
| if (addr) | |
| { | |
| d->sinkMacAddress = QString::fromUtf8(addr); | |
| pa_threaded_mainloop_signal(d->mainloop, 0); | |
| } | |
| }; | |
| auto callback = [](pa_context *c, const pa_sink_info *info, int eol, void *userdata) | |
| { | |
| CallbackData *d = static_cast<CallbackData*>(userdata); | |
| if (eol > 0) | |
| { | |
| pa_threaded_mainloop_signal(d->mainloop, 0); | |
| return; | |
| } | |
| const char *addr = pa_proplist_gets( | |
| info->proplist, | |
| "device.string" | |
| ); | |
| if (addr) | |
| { | |
| d->sinkMacAddress = QString::fromUtf8(addr); | |
| } | |
| // Always signal, even if device.string is not present | |
| pa_threaded_mainloop_signal(d->mainloop, 0); | |
| }; |
🤖 Prompt for AI Agents
In linux/media/pulseaudiocontroller.cpp around lines 128 to 147, the pa_context
callback currently only signals the pa_threaded_mainloop when eol>0 or when the
"device.string" property exists, which can leave waitForOperation hanging if the
property is absent; update the callback so it always signals the mainloop before
returning: after checking eol and after retrieving addr, ensure that in the
branch where addr is null you still call
pa_threaded_mainloop_signal(d->mainloop, 0) (and then return), so every code
path signals the mainloop exactly once.
1 participant
#418
The latest wireplumber version appears to break the MediaController class's "isActiveOutputDeviceAirPods" check, affecting features such as ear detection and conversational awareness. This appears to be due to a name formatting change when querying the default sink name.
Steps to reproduce in the current version:
This PR fixes the issue.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.