[pulseaudio-discuss] [PATCH 2/4] move stream after default sink is changed.

[Hui Wang]

On 2018/12/13 下午5:16, Tanu Kaskinen wrote:
> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>> When default sink changes, the stream should be moved to new
>> default_sink too.
> Except if the stream's preferred sink is the old default sink.
>
>> If it is user to call change default function,
>> all stream will move to new default sink unconditionally; if it
>> is not, will check if stream binds to old_default_sink and the
>> active_port staus of old_default_sink, then it will move the
>> stream conditionally.
> Why does it matter if the default sink changed due to user request or
> some other reason? I don't think streams should be moved
> unconditionally when the user changes the default sink.

Supposing  the sink0 is hdmi, sink1 is analog-speaker,  and a music is 
playing on sink0 (default_sink is sink0),  if users select the 
analog-speaker from UI (gnome-sound-setting), the sink1 is default_sink 
now, and the music should be played on sink1.  If the streams don't move 
to new default_sink unconditionally, I have no idea how to let music be 
played on sink1 here.


For the rest comments, I will fix them as suggested.


Thanks.


>
> I think it would be logical to not care about the port unavailability
> in this patch, because there's a separate patch for handling that.
>
>> diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
>> index faa0f289f..4f01c701f 100644
>> --- a/src/modules/module-switch-on-connect.c
>> +++ b/src/modules/module-switch-on-connect.c
>> @@ -100,29 +97,8 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
>>               return PA_HOOK_OK;
>>           }
>>   
>> -    old_default_sink = c->default_sink;
>> -
>>       /* Actually do the switch to the new sink */
> This comment isn't very helpful any more (not sure how helpful it was
> before either), I'd remove it.
>
>> -    pa_core_set_configured_default_sink(c, sink->name);
>> -
>> -    /* Now move all old inputs over */
>> -    if (pa_idxset_size(old_default_sink->inputs) <= 0) {
>> -        pa_log_debug("No sink inputs to move away.");
>> -        return PA_HOOK_OK;
>> -    }
>> -
>> -    PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
>> -        if (i->preferred_sink != NULL || !PA_SINK_INPUT_IS_LINKED(i->state))
>> -            continue;
>> -
>> -        if (pa_sink_input_move_to(i, sink, false) < 0)
>> -            pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
>> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
>> -        else
>> -            pa_log_info("Successfully moved sink input %u \"%s\" to %s.", i->index,
>> -                        pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), sink->name);
>> -    }
>> -
>> +    pa_core_set_configured_default_sink(c, sink->name, false);
>>       return PA_HOOK_OK;
>>   }
>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>> index 566367f9e..63a3456e7 100644
>> --- a/src/pulsecore/sink.c
>> +++ b/src/pulsecore/sink.c
>> @@ -3886,3 +3886,36 @@ void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume) {
>>       pa_subscription_post(s->core, PA_SUBSCRIPTION_EVENT_SINK|PA_SUBSCRIPTION_EVENT_CHANGE, s->index);
>>       pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_VOLUME_CHANGED], s);
>>   }
>> +
>> +void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *new_sink, bool from_user) {
> I think "pa_sink_move_streams_to_default_sink" would be a better name
> for the function. It would also be good to have a comment explaining
> when the function is used and which streams are not moved (that could
> go to the header file).
>
>> +    pa_sink_input *i;
>> +    uint32_t idx;
>> +    pa_device_port *o_active_p;
>> +
>> +    if (old_sink == new_sink)
>> +	return;
>> +
>> +    if (old_sink == NULL)
>> +	return;
> It doesn't make sense to call this function with NULL, so this check
> should be an assertion instead.
>
>> +
>> +    o_active_p = old_sink->active_port;
> Note that not all sinks have ports, so this can be NULL.
>
> Since we only care about the availability of the active port, I'd add a
> "old_sink_is_unavailable" boolean to be used inside the loop.
>
>> +    if (pa_idxset_size(old_sink->inputs) > 0) {
>> +	    PA_IDXSET_FOREACH(i, old_sink->inputs, idx) {
>> +		if (!PA_SINK_INPUT_IS_LINKED(i->state))
>> +		    continue;
>> +
>> +		if (!from_user && i->preferred_sink != NULL && pa_safe_streq(old_sink->name, i->preferred_sink))
> No need to check if i->preferred_sink is NULL, because pa_safe_streq()
> checks that anyway.
>
>> +		    if (o_active_p->available != PA_AVAILABLE_NO)
>> +			continue;
>> +
>> +		if (pa_sink_input_move_to(i, new_sink, from_user) < 0)
>> +		    pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
>> +				pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), new_sink->name);
>> +		else
>> +		    pa_log_info("Successfully moved sink input %u \"%s\" to %s.", i->index,
>> +				pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_APPLICATION_NAME)), new_sink->name);
> Logging the reason for moving the stream before calling
> pa_sink_input_move_to() would be very useful.
>
> Here's a suggestion you don't need to implement, but you can if you
> want: it would be nice to have good enough logging in
> pa_sink_input_move_finish() so that callers don't all need to do their
> own logging about the move result. This should be done in a separate
> patch.
>