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?
> 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 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?
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?
> 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?
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
