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