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

[Tanu Kaskinen]

On Tue, 2013-11-26 at 12:55 +0100, David Henningsson wrote:
> 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.

Solving problems with few patches would be great, but I don't see paths
to that outcome (other than to squash patches together, which benefits
nobody). The "nearest problem" that I see is to do something useful with
the capability to connect a stream node and a port node, either a client
API for doing that or some kind of an automatic policy (ideas for a
simple but useful policy would be welcome). I suspect that that goal is
more than 10 patches away.

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

Here's the list of new core concepts that I proposed in the Edinburgh
presentation:

 * Router
 * Node
 * Routing group
 * Connection
 * Domain
 * Routing plan
 * Explicit connection request
 * Node features (more likely called Connection requirements)
 * Planned connection

You have seen pa_router and pa_node, and it looks like pa_router will be
dropped. A new addition is pa_device_prototype. The current plan is to
put pa_routing_group, pa_routing_plan, pa_explicit_connection_request
and pa_planned_connection into the Murphy module until there's need for
those outside the Murphy module, so what's still missing from the core
is pa_connection, pa_domain and pa_connection_requirements. Here's
pa_connection (will perhaps be renamed to pa_edge or
pa_node_connection):

struct pa_connection {
    pa_core *core;
    pa_node *input;
    pa_node *output;
    char *string;
};

At this point (when the implementation is missing) I don't think
stream-to-sink and stream-to-port connections will need any further
data, loopback and combine connections will need something when those
connection types become supported.

I can do useful stuff without domains, and adding domains will be pretty
big refactoring, because connections will be subclassed by domains and
nodes will have to deal with being part of multiple domains (so a single
node type like "port" or "sink input" won't be sufficient, because a
node can be e.g. both a port and a module-defined type). I'd rather not
go too much into thinking about the domain details before the
stream-to-port routing is working.

pa_connection_requirements is pretty much open how it will look like. I
think the first thing we want to do with that is to make
stream-to-bluetooth-node routing work so that an appropriate profile is
selected automatically based on the stream type. On what basis should we
do that selection? Phone streams should use HSP and music streams should
use A2DP, but should the HSP profile describe itself as "suitable for
phone audio", or should the HSP profile describe the attributes that
make it somehow more suitable for phone audio than A2DP? I like the
latter more, but in this specific case I think that approach would be
too complicated. The main reason why HSP should be preferred for phone
streams is that it's full duplex, but streams appear one at a time, and
if the phone downlink stream appears first, the policy is going to have
hard time somehow taking into account the uplink stream that doesn't
exist yet, and if it doesn't take it into account, then being a
full-duplex profile has no value. Lower latency would be another reason
to prefer HSP, but I'm not actually convinced that HSP has lower latency
than A2DP, a clarification from someone that knows for sure would be
welcome. The latency constants in module-bluez4-device.c actually
suggest that HSP has higher playback latency than A2DP. So, if we go
with the simpler stream role based policy instead,
pa_connection_requirements would look like this:

struct pa_connection_requirements {
    pa_media_role_t suitable_for_media_role;
};

(Yes, I'd like to have an enumeration for media roles, but we can go
with strings too.)

Setting suitable_for_media_role in pa_connection_requirements would
require the routing target to be explicitly marked as suitable for
routees with that role.

So, now you know what structs I propose for the stage where we can start
to have some benefits for users.

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

pa_node_connection is less vague, but I also like the consistency of
having pa_edge as a counterpart for pa_node. I'd go with pa_edge.

> Anyway, I do see the use of a connection object so I think it's okay to
> keep it.

Cool!

-- 
Tanu