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

[poljar (Damir Jelić)]

Hi Tanu. Thanks for the review.

On Wed, Mar 27, 2013 at 03:22:32PM +0200, Tanu Kaskinen wrote:
> 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.
> 

Heh. I actually did this and then looked up what the definition for
UNKNOWN is and removed it :P

> > +}
> > +
> > +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
> 

The second solution sounds better to me. If we're cleaning up this stuff we
should do it properly. Isn't the feature freeze a problem here? (also I
have an exam next week but I should find time to do this anyway)

> > +
> > +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
>