[3/4] move streams to new appeared sinks if they prefer these sinks

Submitted by Hui Wang on Nov. 5, 2018, 1:47 a.m.

Details

Message ID 20181105014716.5613-4-hui.wang@canonical.com
State Superseded
Headers show
Series "The tentative fix of the issue #185" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Hui Wang Nov. 5, 2018, 1:47 a.m.
When a new sink appears, all streams that have their "preferred_sink"
set to the new sink should be moved to the new sink.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 src/pulsecore/sink.c | 31 +++++++++++++++++++++++++++++++
 src/pulsecore/sink.h |  1 +
 2 files changed, 32 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 63a3456e7..a2a390beb 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -722,6 +722,8 @@  void pa_sink_put(pa_sink* s) {
     /* This function must be called after the PA_CORE_HOOK_SINK_PUT hook,
      * because module-switch-on-connect needs to know the old default sink */
     pa_core_update_default_sink(s->core, false);
+
+    pa_sink_bind_preferred_stream_to_a_sink(s);
 }
 
 /* Called from main context */
@@ -3919,3 +3921,32 @@  void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *ne
 
     return;
 }
+
+void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
+    pa_sink_input *si;
+    uint32_t idx;
+    pa_core *c;
+
+    if (!s)
+	return;
+
+    c = s->core;
+    PA_IDXSET_FOREACH(si, c->sink_inputs, idx) {
+        if (si->sink == s)
+            continue;
+
+        /* Skip this sink input if it is connecting a filter sink to
+         * the master */
+        if (si->origin_sink)
+            continue;
+
+        /* It might happen that a stream and a sink are set up at the
+           same time, in which case we want to make sure we don't
+           interfere with that */
+	if (!PA_SINK_INPUT_IS_LINKED(si->state))
+            continue;
+
+        if (si->preferred_sink != NULL && pa_streq(si->preferred_sink, s->name))
+           pa_sink_input_move_to(si, s, false);
+    }
+}
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index f207a094c..24e4678b1 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -562,4 +562,5 @@  void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume);
     pa_assert(pa_thread_mq_get() || !PA_SINK_IS_LINKED((s)->state))
 
 void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *new_sink, bool from_user);
+void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s);
 #endif

Comments

On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> When a new sink appears, all streams that have their "preferred_sink"
> set to the new sink should be moved to the new sink.
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  src/pulsecore/sink.c | 31 +++++++++++++++++++++++++++++++
>  src/pulsecore/sink.h |  1 +
>  2 files changed, 32 insertions(+)

The sink_put_hook_callback() function in module-stream-restore.c is
obsolete after this change, so it should be removed.

> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 63a3456e7..a2a390beb 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
>      /* This function must be called after the PA_CORE_HOOK_SINK_PUT hook,
>       * because module-switch-on-connect needs to know the old default sink */
>      pa_core_update_default_sink(s->core, false);
> +
> +    pa_sink_bind_preferred_stream_to_a_sink(s);
>  }
>  
>  /* Called from main context */
> @@ -3919,3 +3921,32 @@ void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *ne
>  
>      return;
>  }
> +
> +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {

"Bind" is new terminology, and I'd like to avoid introducing new
terminology if possible. Also, "preferred stream" as a term doesn't
really make sense. So some better name for the function would be
desirable, but I can't immediately think of any obvious names... The
function is about moving streams to a sink that just became available.
"Move" and "streams" should be included in the name... Maybe
"pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
it away from the pa_sink namespace, because the function operates on
streams, not on a sink. It could be added to the pa_sink_input
namespace, but since the function operates on multiple streams rather
than just one, I think pa_core would be a bit better namespace (but I
don't have a very strong opinion on this).

> +    pa_sink_input *si;
> +    uint32_t idx;
> +    pa_core *c;
> +
> +    if (!s)

This should be an assertion.

> +	return;
> +
> +    c = s->core;
> +    PA_IDXSET_FOREACH(si, c->sink_inputs, idx) {
> +        if (si->sink == s)
> +            continue;
> +
> +        /* Skip this sink input if it is connecting a filter sink to
> +         * the master */
> +        if (si->origin_sink)
> +            continue;

I think it't better to ensure that preferred_sink is not set when it
doesn't make sense (and it doesn't make sense with filter sinks, at
least in most cases). So rather than checking si->origin_sink here,
it's better to check this in the code that sets preferred_sink. There
are two situations where preferred_sink is set: when the user moves a
stream, and when module-stream-restore restores the user choice. It
should be sufficient to check this when the user moves the stream,
because module-stream-restore only replicates the moves done by the
user.

> +
> +        /* It might happen that a stream and a sink are set up at the
> +           same time, in which case we want to make sure we don't
> +           interfere with that */
> +	if (!PA_SINK_INPUT_IS_LINKED(si->state))
> +            continue;
> +
> +        if (si->preferred_sink != NULL && pa_streq(si->preferred_sink, s->name))

pa_safe_streq() can be used to avoid the NULL check.

> +           pa_sink_input_move_to(si, s, false);
> +    }
> +}
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index f207a094c..24e4678b1 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -562,4 +562,5 @@ void pa_sink_set_reference_volume_direct(pa_sink *s, const pa_cvolume *volume);
>      pa_assert(pa_thread_mq_get() || !PA_SINK_IS_LINKED((s)->state))
>  
>  void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *new_sink, bool from_user);
> +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s);
>  #endif
On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> > diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> > index 63a3456e7..a2a390beb 100644
> > --- a/src/pulsecore/sink.c
> > +++ b/src/pulsecore/sink.c
> > @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
> >      /* This function must be called after the PA_CORE_HOOK_SINK_PUT hook,
> >       * because module-switch-on-connect needs to know the old default sink */
> >      pa_core_update_default_sink(s->core, false);
> > +
> > +    pa_sink_bind_preferred_stream_to_a_sink(s);
> >  }
> >  
> >  /* Called from main context */
> > @@ -3919,3 +3921,32 @@ void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *ne
> >  
> >      return;
> >  }
> > +
> > +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
> 
> "Bind" is new terminology, and I'd like to avoid introducing new
> terminology if possible. Also, "preferred stream" as a term doesn't
> really make sense. So some better name for the function would be
> desirable, but I can't immediately think of any obvious names... The
> function is about moving streams to a sink that just became available.
> "Move" and "streams" should be included in the name... Maybe
> "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
> it away from the pa_sink namespace, because the function operates on
> streams, not on a sink.

That last sentence can be objected to - the function takes the sink as
an argument, so it can be said to operate on the sink. But then the
verb should be something that the sink does, like "take" ("move" is
something that streams do). So pa_sink_take_streams()? Or
pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
at this point is pa_core_move_streams_to_newly_available_sink(). It's
quite descriptive, but unfortunately lacks the distinction that only
those streams that prefer the newly available sink are moved.
On 2019/1/1 上午2:10, Tanu Kaskinen wrote:
> On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
>> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>> index 63a3456e7..a2a390beb 100644
>>> --- a/src/pulsecore/sink.c
>>> +++ b/src/pulsecore/sink.c
>>> @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
>>>       /* This function must be called after the PA_CORE_HOOK_SINK_PUT hook,
>>>        * because module-switch-on-connect needs to know the old default sink */
>>>       pa_core_update_default_sink(s->core, false);
>>> +
>>> +    pa_sink_bind_preferred_stream_to_a_sink(s);
>>>   }
>>>   
>>>   /* Called from main context */
>>> @@ -3919,3 +3921,32 @@ void pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink, pa_sink *ne
>>>   
>>>       return;
>>>   }
>>> +
>>> +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
>> "Bind" is new terminology, and I'd like to avoid introducing new
>> terminology if possible. Also, "preferred stream" as a term doesn't
>> really make sense. So some better name for the function would be
>> desirable, but I can't immediately think of any obvious names... The
>> function is about moving streams to a sink that just became available.
>> "Move" and "streams" should be included in the name... Maybe
>> "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
>> it away from the pa_sink namespace, because the function operates on
>> streams, not on a sink.
> That last sentence can be objected to - the function takes the sink as
> an argument, so it can be said to operate on the sink. But then the
> verb should be something that the sink does, like "take" ("move" is
> something that streams do). So pa_sink_take_streams()? Or
> pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
> at this point is pa_core_move_streams_to_newly_available_sink(). It's
> quite descriptive, but unfortunately lacks the distinction that only
> those streams that prefer the newly available sink are moved.
>
Understand what you mean here, will have a try to provide a new function 
name and address the rest comments of this patch.

Thanks.
On 1/2/19 7:55 AM, Hui Wang wrote:
> On 2019/1/1 上午2:10, Tanu Kaskinen wrote:
>> On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
>>> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>> index 63a3456e7..a2a390beb 100644
>>>> --- a/src/pulsecore/sink.c
>>>> +++ b/src/pulsecore/sink.c
>>>> @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
>>>>       /* This function must be called after the
>>>> PA_CORE_HOOK_SINK_PUT hook,
>>>>        * because module-switch-on-connect needs to know the old
>>>> default sink */
>>>>       pa_core_update_default_sink(s->core, false);
>>>> +
>>>> +    pa_sink_bind_preferred_stream_to_a_sink(s);
>>>>   }
>>>>     /* Called from main context */
>>>> @@ -3919,3 +3921,32 @@ void
>>>> pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink,
>>>> pa_sink *ne
>>>>         return;
>>>>   }
>>>> +
>>>> +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
>>> "Bind" is new terminology, and I'd like to avoid introducing new
>>> terminology if possible. Also, "preferred stream" as a term doesn't
>>> really make sense. So some better name for the function would be
>>> desirable, but I can't immediately think of any obvious names... The
>>> function is about moving streams to a sink that just became available.
>>> "Move" and "streams" should be included in the name... Maybe
>>> "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
>>> it away from the pa_sink namespace, because the function operates on
>>> streams, not on a sink.
>> That last sentence can be objected to - the function takes the sink as
>> an argument, so it can be said to operate on the sink. But then the
>> verb should be something that the sink does, like "take" ("move" is
>> something that streams do). So pa_sink_take_streams()? Or
>> pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
>> at this point is pa_core_move_streams_to_newly_available_sink(). It's
>> quite descriptive, but unfortunately lacks the distinction that only
>> those streams that prefer the newly available sink are moved.
>>
> Understand what you mean here, will have a try to provide a new
> function name and address the rest comments of this patch.
>
> Thanks.
>
>
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Don't know the code at all, but from a purely English standpoint, how about

"pa_sink_move_streams_to_newly_available_preferred_sink()"

if it's not too impossibly long for a name.
On Wed, 2019-01-02 at 08:25 -0500, Joe wrote:
> On 1/2/19 7:55 AM, Hui Wang wrote:
> > On 2019/1/1 上午2:10, Tanu Kaskinen wrote:
> > > On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
> > > > On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> > > > > diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> > > > > index 63a3456e7..a2a390beb 100644
> > > > > --- a/src/pulsecore/sink.c
> > > > > +++ b/src/pulsecore/sink.c
> > > > > @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
> > > > >       /* This function must be called after the
> > > > > PA_CORE_HOOK_SINK_PUT hook,
> > > > >        * because module-switch-on-connect needs to know the old
> > > > > default sink */
> > > > >       pa_core_update_default_sink(s->core, false);
> > > > > +
> > > > > +    pa_sink_bind_preferred_stream_to_a_sink(s);
> > > > >   }
> > > > >     /* Called from main context */
> > > > > @@ -3919,3 +3921,32 @@ void
> > > > > pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink,
> > > > > pa_sink *ne
> > > > >         return;
> > > > >   }
> > > > > +
> > > > > +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
> > > > "Bind" is new terminology, and I'd like to avoid introducing new
> > > > terminology if possible. Also, "preferred stream" as a term doesn't
> > > > really make sense. So some better name for the function would be
> > > > desirable, but I can't immediately think of any obvious names... The
> > > > function is about moving streams to a sink that just became available.
> > > > "Move" and "streams" should be included in the name... Maybe
> > > > "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
> > > > it away from the pa_sink namespace, because the function operates on
> > > > streams, not on a sink.
> > > That last sentence can be objected to - the function takes the sink as
> > > an argument, so it can be said to operate on the sink. But then the
> > > verb should be something that the sink does, like "take" ("move" is
> > > something that streams do). So pa_sink_take_streams()? Or
> > > pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
> > > at this point is pa_core_move_streams_to_newly_available_sink(). It's
> > > quite descriptive, but unfortunately lacks the distinction that only
> > > those streams that prefer the newly available sink are moved.
> > > 
> > Understand what you mean here, will have a try to provide a new
> > function name and address the rest comments of this patch.
> > 
> > Thanks.
> 
> Don't know the code at all, but from a purely English standpoint, how about
> 
> "pa_sink_move_streams_to_newly_available_preferred_sink()"
> 
> if it's not too impossibly long for a name.

I'd be fine with that. It has the issue that the newly available sink
may not actually be preferred by any stream, so it's slightly
misleading, but it's nevertheless one of the better alternatives, if
not the best so far.
On 07.01.19 18:07, Tanu Kaskinen wrote:
> On Wed, 2019-01-02 at 08:25 -0500, Joe wrote:
>> On 1/2/19 7:55 AM, Hui Wang wrote:
>>> On 2019/1/1 上午2:10, Tanu Kaskinen wrote:
>>>> On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
>>>>> On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
>>>>>> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
>>>>>> index 63a3456e7..a2a390beb 100644
>>>>>> --- a/src/pulsecore/sink.c
>>>>>> +++ b/src/pulsecore/sink.c
>>>>>> @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
>>>>>>        /* This function must be called after the
>>>>>> PA_CORE_HOOK_SINK_PUT hook,
>>>>>>         * because module-switch-on-connect needs to know the old
>>>>>> default sink */
>>>>>>        pa_core_update_default_sink(s->core, false);
>>>>>> +
>>>>>> +    pa_sink_bind_preferred_stream_to_a_sink(s);
>>>>>>    }
>>>>>>      /* Called from main context */
>>>>>> @@ -3919,3 +3921,32 @@ void
>>>>>> pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink,
>>>>>> pa_sink *ne
>>>>>>          return;
>>>>>>    }
>>>>>> +
>>>>>> +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
>>>>> "Bind" is new terminology, and I'd like to avoid introducing new
>>>>> terminology if possible. Also, "preferred stream" as a term doesn't
>>>>> really make sense. So some better name for the function would be
>>>>> desirable, but I can't immediately think of any obvious names... The
>>>>> function is about moving streams to a sink that just became available.
>>>>> "Move" and "streams" should be included in the name... Maybe
>>>>> "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
>>>>> it away from the pa_sink namespace, because the function operates on
>>>>> streams, not on a sink.
>>>> That last sentence can be objected to - the function takes the sink as
>>>> an argument, so it can be said to operate on the sink. But then the
>>>> verb should be something that the sink does, like "take" ("move" is
>>>> something that streams do). So pa_sink_take_streams()? Or
>>>> pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
>>>> at this point is pa_core_move_streams_to_newly_available_sink(). It's
>>>> quite descriptive, but unfortunately lacks the distinction that only
>>>> those streams that prefer the newly available sink are moved.
>>>>
>>> Understand what you mean here, will have a try to provide a new
>>> function name and address the rest comments of this patch.
>>>
>>> Thanks.
>> Don't know the code at all, but from a purely English standpoint, how about
>>
>> "pa_sink_move_streams_to_newly_available_preferred_sink()"
>>
>> if it's not too impossibly long for a name.
> I'd be fine with that. It has the issue that the newly available sink
> may not actually be preferred by any stream, so it's slightly
> misleading, but it's nevertheless one of the better alternatives, if
> not the best so far.
>
How about pa_sink_move_stream_to_new_sink_if_preferred()?
On Mon, 2019-01-07 at 22:20 +0100, Georg Chini wrote:
> On 07.01.19 18:07, Tanu Kaskinen wrote:
> > On Wed, 2019-01-02 at 08:25 -0500, Joe wrote:
> > > On 1/2/19 7:55 AM, Hui Wang wrote:
> > > > On 2019/1/1 上午2:10, Tanu Kaskinen wrote:
> > > > > On Mon, 2018-12-31 at 20:01 +0200, Tanu Kaskinen wrote:
> > > > > > On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> > > > > > > diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> > > > > > > index 63a3456e7..a2a390beb 100644
> > > > > > > --- a/src/pulsecore/sink.c
> > > > > > > +++ b/src/pulsecore/sink.c
> > > > > > > @@ -722,6 +722,8 @@ void pa_sink_put(pa_sink* s) {
> > > > > > >        /* This function must be called after the
> > > > > > > PA_CORE_HOOK_SINK_PUT hook,
> > > > > > >         * because module-switch-on-connect needs to know the old
> > > > > > > default sink */
> > > > > > >        pa_core_update_default_sink(s->core, false);
> > > > > > > +
> > > > > > > +    pa_sink_bind_preferred_stream_to_a_sink(s);
> > > > > > >    }
> > > > > > >      /* Called from main context */
> > > > > > > @@ -3919,3 +3921,32 @@ void
> > > > > > > pa_sink_move_streams_from_oldsink_to_newsink(pa_sink *old_sink,
> > > > > > > pa_sink *ne
> > > > > > >          return;
> > > > > > >    }
> > > > > > > +
> > > > > > > +void pa_sink_bind_preferred_stream_to_a_sink(pa_sink *s) {
> > > > > > "Bind" is new terminology, and I'd like to avoid introducing new
> > > > > > terminology if possible. Also, "preferred stream" as a term doesn't
> > > > > > really make sense. So some better name for the function would be
> > > > > > desirable, but I can't immediately think of any obvious names... The
> > > > > > function is about moving streams to a sink that just became available.
> > > > > > "Move" and "streams" should be included in the name... Maybe
> > > > > > "pa_sink_move_streams_to_newly_available_sink()"? I also suggest moving
> > > > > > it away from the pa_sink namespace, because the function operates on
> > > > > > streams, not on a sink.
> > > > > That last sentence can be objected to - the function takes the sink as
> > > > > an argument, so it can be said to operate on the sink. But then the
> > > > > verb should be something that the sink does, like "take" ("move" is
> > > > > something that streams do). So pa_sink_take_streams()? Or
> > > > > pa_sink_take_streams_that_prefer_it()? I don't know. My weak preference
> > > > > at this point is pa_core_move_streams_to_newly_available_sink(). It's
> > > > > quite descriptive, but unfortunately lacks the distinction that only
> > > > > those streams that prefer the newly available sink are moved.
> > > > > 
> > > > Understand what you mean here, will have a try to provide a new
> > > > function name and address the rest comments of this patch.
> > > > 
> > > > Thanks.
> > > Don't know the code at all, but from a purely English standpoint, how about
> > > 
> > > "pa_sink_move_streams_to_newly_available_preferred_sink()"
> > > 
> > > if it's not too impossibly long for a name.
> > I'd be fine with that. It has the issue that the newly available sink
> > may not actually be preferred by any stream, so it's slightly
> > misleading, but it's nevertheless one of the better alternatives, if
> > not the best so far.
> > 
> How about pa_sink_move_stream_to_new_sink_if_preferred()?

That sounds very good to me (just replace "stream" with "streams").