On 11/14/2011 10:31 AM, Colin Guthrie wrote:
'Twas brillig, and David Henningsson at 14/11/11 07:32 did gyre and gimble:
On 11/13/2011 03:42 PM, Colin Guthrie wrote:
Hi,
I've written up my latest proposal to gather feedback before starting
(hopefully soon now) on the implementation.
http://www.freedesktop.org/wiki/Software/PulseAudio/RFC/PriorityRouting
Nice!
Comments most welcome. Don't forget to have a quick look at the patch
linked in the introduction before looking at the example code chunks in
the wiki page.
I'm particularly interested to get feedback from the embedded folks as
I'm very much focussed on the desktop use cases but obviously would like
a system that would benefit other use cases too or at very least didn't
hinder them!
First; as you already know, I very much support an improved routing
system and so this is mostly about details.
+typedef struct {
+ pa_stream_direction_t direction;
+ pa_proplist *proplist;
+ union {
+ pa_sink *sink;
+ pa_source *source;
+ } device;
+
+ union {
+ pa_sink *sink;
+ pa_source *source;
+ } ignore;
+} pa_route_decision_t;
1) Nitpick: I think it's more common with "typedef struct
pa_route_decision"
Noted.
2) I'm a little confused that you don't send a pointer to the actual
sink input, only its proplist. Why?
Well two reasons really.
Firstly I originally wanted the stuct here to be as direction agnostic
as possible... I kinda ruined that but putting in specific pointers for
sink/source and the ignore_sink/source but the aim is still hiding in
there when possible. Ideally I'd like to replace the sink/source
pointers, but I'm not sure what's cleaner - stronger typing with maybe a
few more if/else or more generic structs... Thoughts welcome!
The second, and arguably more valid, is that this function is also
called before the pa_sink_input pointer is ready (i.e. with
sink_input_new_data. In this case I simply don't have such a pointer to
pass in, but I do have it's proplist.
There is however still some degree of chicken and egg stuff here. At
present I call the routing hook PA_CORE_HOOK_SINK_INPUT_ROUTE before the
new hook PA_CORE_HOOK_SINK_INPUT_NEW where other modules that might
populate the new_data's proplist which in turn may have helped the
routing decsions (e.g. augment properties etc).
I did this so as to prevent e.g. module-stream-restore from setting the
sink naively before the "real" routing decision, but in practice this
will likely have change and we'll just ensure that modules (like
stream-restore) do not set the sink at this stage and do the routing
after the new_data's proplist is tweaked i.e. switch the call order of
PA_CORE_HOOK_SINK_INPUT_ROUTE and PA_CORE_HOOK_SINK_INPUT_NEW.
Makes sense...I think :-)
My point is that routing modules is likely to want to use as much
information as possible, and even if "information hiding" is a good
principle in general, I'm not sure that applies to routing modules.
Therefore we might be better off with supplying the sink input pointer
instead of just the proplist (or both, if you want).
Thanks for making me think about this in more depth :)
3) union between sink and source - we don't lack memory here, so might
be safer to have two different fields instead of a union. Reduces the
risk of unpredictable segfaults in favour of predictable ones.
Yeah, this is just how the evolution of thought process worked rather
than any specific design ("I want a single pointer here... hmm, that's
not easy, I guess I'll need two... unions are used for that, I'll use a
union"). I agree that in practice it would be neater to ditch the union.
4) Do we really need the ignore field? It is only used in the unlink
phase (right?) and in that case the source/sink should already been in a
state that the routing modules can use to know that they should not
prefer it.
Yeah it's for unlinking, but no, it's not called when the sink is in a
state that routing modules can know about it.
As you can see here:
http://colin.guthr.ie/git/pulseaudio/diff/src/pulsecore/sink.c?h=route&id=61d73564c1c2b0b832f14bfd4f2b77c98febb735
it's called when the sink is still linked. If delay the call until after
we unlink, modules like module-rescue-streams will already have done
their job and selected a new device, which is something we want to
avoid. Also there is some code that iterates through and kills any
streams before the sink is unlinked, so we actually can't delay it too
much anyway.
It's not actually that different to what module-rescue-streams does (it
also has an "skip" sink)
find_evacuation_sink(pa_core *c, pa_sink_input *i, pa_sink *skip);
So I don't think we can avoid this approach.
Hmm. In my world module-rescue-streams would either be completely
replaced by the general routing, or changed to listen to the "route"
hook, as this is essentially a routing module.
It would just have seemed more natural to read everything out of the
sink state, rather to have one sink state and one pointer essentially
saying "don't trust the sink state". But maybe adding another sink state
"unlinking" would be overkill...
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
