On Tue, 2013-03-26 at 21:54 +0100, poljar (Damir Jeli?) wrote:
> Port creation is now slightly different. It is now similar to how
> other objects are created (e.g. sinks/sources/cards).
>
> This should become more useful in the future when we move more stuff to
> the ports.
>
> Functionally nothing has changed.
> ---
> src/modules/alsa/alsa-mixer.c | 8 +++-
> src/modules/alsa/alsa-ucm.c | 9 +++-
> src/modules/bluetooth/module-bluetooth-device.c | 26 ++++++++----
> src/pulsecore/device-port.c | 56 +++++++++++++++++++++----
> src/pulsecore/device-port.h | 17 +++++++-
> 5 files changed, 97 insertions(+), 19 deletions(-)
> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index c877df2..aba9ac2 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -2070,6 +2070,8 @@ off:
> /* Run from main thread */
> static void create_card_ports(struct userdata *u, pa_hashmap *ports) {
> pa_device_port *port;
> + pa_device_port_new_data output_port_data, input_port_data;
It doesn't seem necessary to have two separate structs.
> diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
> index f16de3a..7438dec 100644
> --- a/src/pulsecore/device-port.c
> +++ b/src/pulsecore/device-port.c
> @@ -26,6 +26,46 @@
>
> PA_DEFINE_PUBLIC_CLASS(pa_device_port, pa_object);
>
> +pa_device_port_new_data *pa_device_port_new_data_init(pa_device_port_new_data *data) {
> + pa_assert(data);
> +
> + pa_zero(*data);
> + return data;
I think it would be good to explicitly initialize available to
PA_AVAILABLE_UNKNOWN. Sure, UNKNOWN is defined as zero, so there's no
practical effect, but I don't like assuming any knowledge about the
numeric values of enumerations.
> +}
> +
> +void pa_device_port_new_data_set_name(pa_device_port_new_data *data, const char *name) {
> + pa_assert(data);
> +
> + pa_xfree(data->name);
> + data->name = pa_xstrdup(name);
> +}
> +
> +void pa_device_port_new_data_set_description(pa_device_port_new_data *data, const char *descritpion) {
Typo: "descritpion"
> + pa_assert(data);
> +
> + pa_xfree(data->description);
> + data->description = pa_xstrdup(descritpion);
> +}
> +
> +void pa_device_port_new_data_set_availability(pa_device_port_new_data *data, pa_available_t available) {
I think "set_available" would be better, because the field name is
"available".
> + pa_assert(data);
> +
> + data->available = available;
> +}
> +
> +void pa_device_port_new_data_set_direction(pa_device_port_new_data *data, pa_direction_t direction) {
> + pa_assert(data);
> +
> + if (direction == PA_DIRECTION_OUTPUT)
> + data->is_output = true;
> + else if (direction == PA_DIRECTION_INPUT)
> + data->is_output = true;
> +}
I think the direction should be set either always or never with
pa_device_port_new_data_set_direction(). I'd prefer the "always" option,
but since alsa modules can have dual-direction ports, they can't really
use pa_device_port_new_data_set_direction(), so as a second best
alternative, I suggest that you remove this function.
Or there's another alternative: I could push a patch set[1] that removes
the possibility of dual-direction ports. That would possibly require
some rebasing work from you. What do you prefer? Remove
pa_device_port_new_data_set_direction() for now, or rebase on top of the
mentioned patch set after I've applied that?
[1] http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/13010/focus=13626
> +
> +void pa_device_port_new_data_done(pa_device_port_new_data *data) {
> + pa_assert(data);
> +}
name and description should be freed here (and they should be set to
NULL in pa_device_port_new()).
> +
> void pa_device_port_set_available(pa_device_port *p, pa_available_t status)
> {
> pa_core *core;
> @@ -66,23 +106,25 @@ static void device_port_free(pa_object *o) {
> }
>
>
> -pa_device_port *pa_device_port_new(pa_core *c, const char *name, const char *description, size_t extra) {
> +pa_device_port *pa_device_port_new(pa_core *c, pa_device_port_new_data *data, size_t extra) {
> pa_device_port *p;
>
> - pa_assert(name);
> + pa_assert(data);
pa_assert(data->name) would be good to have too.
And also pa_assert(data->direction == PA_DIRECTION_INPUT ||
data->direction == PA_DIRECTION_OUTPUT), if the direction is to be
initialized in the new_data.
--
Tanu
