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
