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
>
