[pulseaudio-discuss] [PATCH 14/21] port-node: Introduce pa_port_node

[Tanu Kaskinen]

On Thu, 2013-06-20 at 06:22 +0200, David Henningsson wrote:
> On 06/19/2013 08:11 PM, Tanu Kaskinen wrote:
> > On Wed, 2013-06-19 at 19:50 +0200, David Henningsson wrote:
> >> On 06/19/2013 05:40 PM, Tanu Kaskinen wrote:
> >>> pa_port_node takes care of things that are common to all port nodes.
> >>> The port device class is used in the node name for some extra
> >>> user-friendliness. The goal is to avoid the long and cryptic names
> >>> that are currently generated for sinks, sources and cards.
> >>
> >> I'm not following. Why do we need extra pa_port_node and pa_sink_node
> >> objects? Why don't we just have a pa_node pointer (or even embedded
> >> struct, if that makes sense) in pa_device_port / pa_sink instead?
> >
> > pa_node will probably have some callbacks, at least for activating the
> > node. I expect that e.g. pa_port_node would have a common implementation
> > for activating ports. There may or may not be need for further
> > specializing the activation code by adding an activation callback also
> > to pa_port_node. This is largely speculation, however, because this
> > hasn't been fully designed yet.
> 
> Hmm, I'm still not seeing why all types of specialization and callbacks 
> can't just be handled by code taking just a node pointer, like:
> 
> /* In device-port.c */
> 
> void activate_node_cb(pa_node *n)
> {
>     pa_device_port *p = n->userdata;
> 
>     /* Do some specialized node related stuff here */
> 
>     activate_port(p);
> }
> 
> If necessary, this method can also call p's callbacks if you need to 
> something differently for alsa and bt (although it would be more elegant 
> if such code was inside activate_port() rather than activate_node_cb).

I think my original answer was just a reflex to defend my choice,
however bad it might have been. After thinking a bit more, I believe the
reason why I chose this approach was that it was the first thing that
came to my mind, given the requirement that "there probably will be code
that is specific to port nodes". Having a class for port nodes then
seemed obvious, but I agree that it seems to be unnecessarily heavy and
complicated.

Instead of implementing the port node callbacks in device-port.c, I'd
like to reserve the callbacks to module code. I propose that any generic
port node code is put to node.c. There will be a node type field that
node.c can use to determine how e.g. activation should be handled. The
type field was in the plans anyway (and I have already implemented it),
it's needed for other purposes too.

> > If you think pa_port_node is bad, we
> > could try going without it until we actually need it.
> 
> At this point the struct looks very superfluous to me.
> 
> >> If every sink, source and port have exactly one node, that seems to be
> >> the logical object model. Or can sinks, sources and ports have more than
> >> one node attached to them?
> >
> > I don't think it's likely that we would some day need one port or sink
> > to have multiple nodes - we certainly don't need it today. It's entirely
> > possible that a port or a sink could have zero nodes, however (or at
> > least it's possible for sinks - e.g. bluetooth sinks don't have nodes,
> > and neither do alsa sinks if ports are present).
> 
> Ok, then the logical object model to me seems to have a pa_node pointer 
> in pa_device_port and pa_sink. That also simplifies code because you 
> don't have to do anything in the alsa/jack/bluetooth backends - just 
> initialize the node in e g pa_sink_new and/or pa_sink_put.

This is one thing to discuss - should nodes be created in
pa_sink_new/put(), or should the backend create the nodes (and if the
latter, at what time)?

The backend needs to control the creation anyway, because not every sink
is going to have a node, so the backend has to decide whether a node
will be created. The backend also needs to customize the node
initialization (at least the node name, but I expect there to be need
for custom callbacks too and possibly other properties). For that reason
it seems better that the backend creates and owns the node.

As for when the backend should create the node: I have so far chosen the
approach that nodes are created after pa_sink_put() (and other similar
put() calls), because it was simple that way. However, at least in case
of streams, the node creation should happen with careful cooperation
with sink input creation, because the routing is decided before
pa_sink_input_put(). If the routing policy operates on nodes, the node
creation must happen before pa_sink_input_put(), otherwise the stream
will be incorrectly routed for a short time.

How to proceed? I will definitely get rid of pa_port_node and friends. I
guess I should also rework the node creation order, since it needs to be
changed anyway at some point. Should sink-input.c call pa_node_new(), or
should the backend do that? As I said, my preference is to do it in the
backend, but do you have different opinion?

-- 
Tanu