[pulseaudio-discuss] [PATCH] device-port: Introduce port_new_data struct.

[Tanu Kaskinen]

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