[pulseaudio-discuss] [PATCH v2 2/9] dbus: Track the default sink and source with hooks

[David Henningsson]

On 03/18/2013 03:39 PM, Tanu Kaskinen wrote:
> On Mon, 2013-03-18 at 14:00 +0100, David Henningsson wrote:
>> On 02/20/2013 07:23 PM, Tanu Kaskinen wrote:
>>> ---
>>>    src/modules/dbus/iface-core.c |  173 +++++++++++++++++------------------------
>>>    1 file changed, 72 insertions(+), 101 deletions(-)
>>>
>>> diff --git a/src/modules/dbus/iface-core.c b/src/modules/dbus/iface-core.c
>>> index a9716bc..4621726 100644
>>> --- a/src/modules/dbus/iface-core.c
>>> +++ b/src/modules/dbus/iface-core.c
>>> @@ -108,9 +108,8 @@ struct pa_dbusiface_core {
>>>        pa_hashmap *modules;
>>>        pa_hashmap *clients;
>>>
>>> -    pa_sink *fallback_sink;
>>> -    pa_source *fallback_source;
>>> -
>>> +    pa_hook_slot *default_sink_changed_slot;
>>> +    pa_hook_slot *default_source_changed_slot;
>>>        pa_hook_slot *sink_put_slot;
>>>        pa_hook_slot *sink_unlink_slot;
>>>        pa_hook_slot *source_put_slot;
>>> @@ -677,13 +676,12 @@ static void handle_get_fallback_sink(DBusConnection *conn, DBusMessage *msg, voi
>>>        pa_assert(msg);
>>>        pa_assert(c);
>>>
>>> -    if (!c->fallback_sink) {
>>> -        pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY,
>>> -                           "There are no sinks, and therefore no fallback sink either.");
>>> +    if (!c->core->default_sink) {
>>> +        pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY, "No fallback sink set.");
>>>            return;
>>>        }
>>
>> Is there a reason for using core->default_sink instead of
>> pa_namereg_get_default_sink?
>
> There's this reason: the property change signals should match what
> getting the property would return, at least in my opinion. If we'd call
> pa_namereg_get_default_sink() here, then the signals would indicate that
> no fallback has been set, but getting the fallback would indicate that
> there's some fallback set.
>
> It would definitely be good to have a comment about this here.

What about caching the result of pa_namereg_get_default_sink in the 
beginning, and only send out a dbus signal if the result of the function 
has actually changed?

>
>> E g, the native protocol (e g pactl stat) uses
>> pa_namereg_get_default_sink instead of the direct pointer.
>>
>> As for the other dbus patches (and "core: Add hooks for default device
>> changes"), I looked them briefly through and saw nothing to remark on.
>> With the disclaimer that I don't know the dbus stuff very well, but I'd
>> just say it's a very welcome fix, if it fixes the occasional crashes on
>> device unplug.
>
> OK, if you ack my explanation above, I'll push patches 1-4 (Arun wants
> to check patch 5 before I push it). Thanks for the review!
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic