'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. 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. Thanks for your comments :) Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited http://www.tribalogic.net/ Open Source: Mageia Contributor http://www.mageia.org/ PulseAudio Hacker http://www.pulseaudio.org/ Trac Hacker http://trac.edgewall.org/
