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

[Tanu Kaskinen]

On Thu, 2013-06-20 at 10:14 +0200, David Henningsson wrote:
> On 06/20/2013 09:29 AM, Tanu Kaskinen wrote:
> > 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.
> 
> What kind of module is this referring to? Policy modules?

No, I mean node backends. Alsa, bluetooth etc.

> > 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.
> 
> I'm okay with this approach too - code can easily be moved from node.c 
> to device-port.c if we think this is better at some other time.
> 
> If we're going to have owner types, this also opens up for inline 
> functions such as
> 
> static inline pa_device_port *node2port(pa_node *n)
> {
>     pa_assert(n->type = PA_NODE_TYPE_DEVICE_PORT);
>     return (pa_device_port *) n->owner;
> }
> 
> (Owner is probably a better name than "userdata" if it always refer to 
> the node owner.)
> 
> I would prefer a vtable over a big switch on "type" though.

If I understand correctly, you refer here to implementing the callbacks
in device-port.c, i.e. the callbacks should not be reserved to module
code (even though you said you'd be OK with that). I can't demonstrate
any problem with it at this point, so I'll do it your way (neither of us
seem to have a strong opinion).

> >>> 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.
> 
> The primary reason I prefer to put it in the port/sink/source rather 
> than the backend is that we get a lot less code duplication between 
> backends.
> If the backend needs to customise something, it could do that, but if it 
> does not, why not have code in the port/sink/source objects that deals 
> with the generic case?

When no customization is needed, the backend code is very simple: just
call a generic function for initializing the node data according to the
node type, call pa_node_new(), and clean up with pa_node_free(). If the
ownership is pushed to port/sink/source, we only save the cleanup, but I
guess that's significant enough to matter, so let's try that.

> We could consider having a node_new_data struct inside 
> port/sink/source_new_data struct, and also a flag for whether to create 
> it or not. What do you think of that?

It should be OK.

-- 
Tanu