On 05.07.19 09:56, Tanu Kaskinen wrote:
On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
On 02.07.19 06:49, Tanu Kaskinen wrote:
On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
On 01.07.19 08:03, Georg Chini wrote:
On 01.07.19 07:08, Tanu Kaskinen wrote:
On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:
On 17.01.19 07:53, Hui Wang wrote:
When the default sink changes, the streams from the old default sink
should be moved to the new default sink, unless the preferred_sink
string is set to the old default sink and the active port of the old
default sink is not unavailable
+
+void pa_sink_move_streams_to_default_sink(pa_core *core, pa_sink
*old_sink, bool from_user) {
+ pa_sink_input *i;
+ uint32_t idx;
+ bool old_sink_is_unavailable;
Does this not give a warning about using uninitialized variables?
+
+ pa_assert(core);
+ pa_assert(old_sink);
+
+ if (old_sink == core->default_sink)
+ return;
+
+ if (old_sink->active_port && old_sink->active_port->available
== PA_AVAILABLE_NO)
+ old_sink_is_unavailable = true;
This is not sufficient to determine if the old sink is still available.
There are sinks
that do not have ports (null-sink, virtual sinks). I think you will
need
another boolean
argument to the function which indicates availability of the old sink.
The definition of an unavailable sink is that its active port is
unavailable. I don't know what kind of sinks you're thinking about
here, maybe virtual sinks that are on top of an unavailable hardware
sink? There are many places where we check the availability of a sink
this way, and I don't think it makes sense to be different here.
I don't understand. Virtual sinks don't have ports. So checking only
sinks that have an active port excludes all sinks that don't have
ports like the null-sink and virtual sinks. Following your definition
above, those sinks are never available.
You have it reversed: following my definition above, those sinks are
always available.
Right, sorry. (I seem to be reading things wrong quite often
the last few weeks ... I'll do my best to be more thorough in
the future.)
Checking the code, only alsa, bluetooth and raop sinks define ports and
the "many places" you are referring to are compare_sinks() and
compare_sources() in core.c and a check in module-switch-on-connect,
which is used to determine the availability of the default sink.
At least module-stream-restore checks device availability too (although
probably not anymore after this patch set, because module-stream-
restore won't do the routing directly anymore, it will just restore the
preferred_sink variable, which can be done regardless of the sink
availability).
Extending the hardware sink availability to filter sinks probably makes
sense, but I think that's a topic for a separate patch (or patches).
You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
which seems unnecessary to me. The function can in any case figure out
the availability by itself (possibly using a helper function
pa_sink_is_available() that can handle filter sinks too).
I think some additional check is still needed at least for
s->unlink_requested
because when a sink is going to be unlinked and the function is called from
pa_sink_unlink() in patch 7, there may be nothing else that indicates
that the
sink is going to be removed. When I proposed an additional argument, I did
not see that pa_sink_unlink() already sets a flag that can be used.
So I would suggest that the helper function checks on
- link state
I don't expect this to be necessary, although it can't hurt either.
- port availability
- s->unlink_requested
If this is indeed used from pa_sink_unlink() to rescue streams, then
checking this seems appropriate (and can't hurt anyway).
- suspend state (any reason apart from SUSPEND_IDLE)
This doesn't seem appropriate, unless we start avoiding suspended sinks
in the same way we avoid unavailable sinks. That would mean that we
would never use a suspended sink as the default sink. That would
probably make sense actually, so I'm just pointing out that we need to
be consistent: if we avoid suspended sinks in one situation, we should
avoid them in all situations.
Yes, we should do that. Therefore my suggestion is to add a helper
function to this patch set. For consistency we could exclude the
suspend state for the moment, though I would recommend to include
it. Using the helper function outside this patch set in all places where
we check availability should then be the task of another patch series.
By suspended I guess you mean suspended apart from idle, correct?
Using the helper function in other places (and extending it if I forgot
something
in the list above) could then be done in a separate patch. Are you OK
with that?
Yes, I think a helper function can be added, and if it's added, it's
added in a separate patch.
I don't really understand what your suggestion means for the patch set
we are discussing. Do you mean the helper should be added to that
set but in a separate patch? Or do you mean it should not be added
to this set? If it is not added, at least an additional check for
s->unlink_requested is required in patch 7.
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
