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
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
