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

[Tanu Kaskinen]

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.

> > 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?

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);

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).

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.

-- 
Tanu