[pulseaudio-discuss] [PATCH 00/11] Routing: Move routing policy code out of module-alsa-card

[David Henningsson]

On 11/25/2013 08:33 AM, Tanu Kaskinen wrote:
> On Mon, 2013-11-25 at 03:55 +0100, David Henningsson wrote:
>> On 11/24/2013 07:48 AM, Tanu Kaskinen wrote:
>>> On Sun, 2013-11-24 at 07:57 +0200, Tanu Kaskinen wrote:
>>>> module-alsa-card does currently a bit of routing: it moves streams
>>>> during profile switches with the goal of keeping streams connected to
>>>> the same card before and after the profile switch. This doesn't
>>>> belong to module-alsa-card, it should be in a policy module, so this
>>>> patch set removes that code from module-alsa-card.
>>>
>>> I'll start writing the next patch set now. To ensure that reviewers are
>>> happy with the scope of the next patch set (there have been problems
>>> with the scope in the past), I'd like potential reviewers to ack or nack
>>> the following plan (the sooner the better):
>>
>> I don't know if I count myself as a potential reviewer at this point;
>> both due to the lack of time, and the general feeling that this is
>> getting too big and complex already, but anyway...
> 
> Thank you anyway for the quick feedback. I can't do anything about the
> lack of time, but I can work on reducing the complexity to a level that
> you're comfortable with as long as you tell me how.

I think what's needed now is try to shorten the path from "current git
master" (main branch, not the routing branch) to "end users see an
improvement". We're currently at 50 (?) patches (counting routing branch
diff and review pending patches) and we're still just building
infrastructure. I want to solve problems.

Another way to do it could be to do the interfaces first and
implementations later; i e, first, present the data structures you want
to add and how they fit together, and get an ack on that before writing
the entire implementation. That could help us agree on the structure
before you spend time on writing all the code.

But hey, I'm not a review expert and this by far the largest thing I
have even started to try to review. So I'm not sure what the best
strategy is.

>>> Currently module-experimental-router sets the initial sink of a sink
>>> input directly, but the routers shouldn't need to care about whether the
>>> nodes are sinks or ports or sink inputs or whatever, and this is what
>>> I'd like to work on next. 
>>
>> My gut feeling is "nak, at least for now".
>>
>> I think that if the router currently sets the sink of a sink input
>> directly, that sounds like a reasonable middle step. So before carrying
>> away even further, let's look at where we are now:
>>
>>  1) Is the current patch set doing anything of value?
> 
> It moves code out of module-alsa-card that doesn't belong there, and the
> patch set implements changes that are useful in adding bigger
> functionality improvements later.
> 
>>  2) Does it add any new functionality that we did not have before?
> 
> I don't think so, unless you count the possibility to get a list of
> nodes in the system.
> 
>>  3) And just a side question, out of personal interest - we've had
>> problems that the sink for a sink input switches during a S3, because
>> the USB or Bluetooth stack is not completely up when PulseAudio returns,
>> so to PulseAudio it looks like the card is disconnected and then
>> connected again.
>> Do you have any thoughts about this problem and how the new routing
>> infrastructure would help?
> 
> I'm not familiar with the problem and it's practical effects to the
> users. Is the problem that streams are moved away from the removed
> devices and not restored once the device comes back? Or does the
> reconnection trigger a "switch-on-connect" event and streams that were
> previously not connected to the device are moved against the user's wish
> to the device? Or both?

The former, I believe. We don't move streams on connect, but we do
"rescue" them on disconnect.

> A solution to those problems would be to prioritize the devices in the
> system, and re-evaluate the stream routing whenever devices are added or
> removed. A new device being plugged in shouldn't affect that device's
> priority, at least if the device has been seen before.
> 
> Now that the plan has changed to not have the routing groups (useful for
> prioritization) and the routing algorithm (useful for doing the stream
> routing planning and execution) in the core, the routing infrastructure
> won't help with those tasks that much, the main thing that the routing
> infrastructure does is to help with the profile and port activation
> (part of the routing planning and execution).
> 
>>> How should the initial sink for a sink input
>>> be set in the brave new node-based world? I think this should happen as
>>> part of creating a new connection object. So, I want to introduce the
>>> connection objects.
>>
>> Also, I don't see how this would help. I mean, I can understand that
>> it's useful to have connection objects (e g to distinguish between
>> automatic and manual connections - this is a property of the
>> connection), but why would connection objects be better candidates to
>> call pa_sink_input_* than routers?
>>
>> Or to put it in another way, you say that "routers shouldn't need to
>> care about whether the nodes are sinks or ports or sink inputs or
>> whatever". I question this claim and wonder why connection objects
>> should be the ones to care instead?
>>
>> If the reason is to avoid code duplication between
>> module-experimental-router and module-murphy-router, then maybe just
>> doing a, say
>>
>> pa_node_connect(pa_node *from_node, pa_node *to_node)
>>
>> ...could deal with performing this internally - it still isn't a reason
>> to add connection objects.
> 
> Yes, avoiding code duplication in router modules is why router modules
> shouldn't concern themselves with the details of connecting different
> kinds of nodes (and once there are node types that aren't part of the
> core, router modules will not be able to connect those by themselves
> anyway).
> 
> I planned to have these functions:
> 
> int pa_core_create_connection(pa_core *core, pa_node *input, pa_node *output, pa_connection **connection);
> void pa_core_delete_connection(pa_core *core, pa_connection *connection);

pa_connection sounds a bit vague; I'd suggest either pa_node_connection
or pa_edge (nodes and edges are both graph terms).

Anyway, I do see the use of a connection object so I think it's okay to
keep it. As long as you think simple and minimalistic about it - what
are the expected struct members in pa_connection (or what it ends up
being named)?

> 
> I can replace those easily enough with
> 
> int pa_node_connect(pa_node *input, pa_node *output);
> void pa_node_disconnect(pa_node *input, pa_node *output);
> 
> I wrote this piece of code for dealing with the case of a sink input
> requesting routing to multiple sinks (or any other case of "route this
> new node to multiple places"):
> 
>     pa_assert(n_initial_connections > 1);
>     created_connections = pa_dynarray_new(NULL);
> 
>     for (i = 0; i < n_initial_connections; i++) {
>         r = pa_core_create_connection(node->core, node, initial_connections[i], &connection);
> 
>         if (r < 0) {
>             PA_DYNARRAY_FOREACH(connection, created_connections, idx) {
>                 pa_log_info("Undoing connection %s.", connection->string);
>                 pa_core_delete_connection(node->core, connection);
>             }
> 
>             goto finish;
>         }
> 
>         pa_dynarray_append(created_connections, connection);
>     }
> 
> The idea here is to implement an all-or-noting policy: either all
> connections are created or none are created.
> 
> I can replace this code with
> 
>     r = -PA_ERR_NOTSUPPORTED;
>     goto finish;
> 
> because the system won't support this use case anyway at this point. And
> now I realized that I could also rely on the fact that if this code
> returns an error, then the node we're dealing with will be removed
> anyway and therefore any connections associated with it will be removed
> too. Anyway, my point here is that this shows a couple of benefits of
> pa_connection objects that are unlikely to be specific to this code:
> 
>     - It's convenient to get a connection object out of
> pa_core_create_connection(), if the code has to track the created
> connections somehow, because the alternative would be to create custom
> node pair objects, which is cumbersome.
>     - (This second point is very minor, but I'll mention it for
> completeness.) pa_connection.string contains a string of this form:
> "some_node -> some_other_node". Printing this kind of strings will be a
> common occurrence, and it would be slightly more cumbersome if it was
> necessary to do this:
> 
>     pa_log_info("Undoing connection %s -> %s.", node1->name, node2->name);
> 
> If connecting two nodes requires e.g. a loopback, the loopback object is
> most naturally owned by a connection object, not by either of the
> connected nodes, so I hope you won't be against connection objects in
> the long term. I can replace the pa_core_create/delete_connection()
> functions with pa_node_connect/disconnect() at this point if you wish.
> 
>>> Who manages and owns the connections? If there's no router, the core
>>> will of course need to manage the connections. The natural owner for
>>> sink input connections in this case is pa_sink_input. If there is a
>>> router, then connections may be created by the router, and I think it
>>> makes sense in that case to make the router also the owner of the
>>> connections, so freeing the connections is then also the responsibility
>>> of the router. Hopefully it won't become too complex to tell apart
>>> connections owned by the core and connections owned by a router.
>>
>> I previously suggested that we would just have a long list of
>> connections in a simply linked list, and that linked list would be added
>> to and removed from by the core, maybe inside the node code.
>>
>> There are no other pointers to these connection objects from anywhere
>> else (this is to prevent dangling pointers) - just a simple lookup
>> function that returns the connection object given a from_node and to_node.
>>
>> It sounds like you instead want to spread memory management of the
>> connection objects all across the place, which sounds very error prone.
> 
> I suppose you hadn't read the mail I sent later yesterday when you wrote
> this. I agree that pa_core should do the memory management of
> connections (until routing domains are added).

Ok, found it now.

> I disagree about the approach of never referencing the connection
> objects outside pa_core, dangling pointers can be avoided just fine
> without that restriction, but I don't think that's an important topic to
> discuss right now, if I'm going to remove the connection objects from
> this patch set.



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic