Hi, Looks quite good to me. Some review comments below. On Wed, 2011-10-12 at 12:40 +0200, David Henningsson wrote: > Note: There is still no notification when status availability changes. > > Signed-off-by: David Henningsson <david.henningsson at canonical.com> > --- > > The protocol skew in Ubuntu 11.10 was actually a mistake on my part. Since the UI > changes that would depend on this information being available was backed out, > I probably should have backed the actual protocol change out as well. > > Anyway, here is the patch that forms one of the base features for jack detection, > and brings upstream out of protocol skew with Ubuntu 11.10. > > (Reposted: Thanks courmisch for finding a typo.) > > PROTOCOL | 7 +++ > configure.ac | 2 +- > src/modules/module-tunnel.c | 91 +++++++++++++++++---------------------- > src/pulse/def.h | 15 ++++++ > src/pulse/introspect.c | 16 +++++++ > src/pulse/introspect.h | 2 + > src/pulsecore/protocol-native.c | 4 ++ > src/pulsecore/sink.h | 1 + > 8 files changed, 85 insertions(+), 53 deletions(-) > > diff --git a/PROTOCOL b/PROTOCOL > index 8c69190..b8b61e4 100644 > --- a/PROTOCOL > +++ b/PROTOCOL > @@ -278,6 +278,13 @@ New field in PA_COMMAND_UNDERFLOW: > > int64_t index > > +## v24, implemented by >= 2.0 > + > +New field in all commands that send/receive port introspection data > +(PA_COMMAND_GET_(SOURCE|SINK)_INFO, PA_COMMAND_GET_(SOURCE|SINK)_INFO_LIST]): > + > + uint32_t available > + > #### If you just changed the protocol, read this > ## module-tunnel depends on the sink/source/sink-input/source-input protocol > ## internals, so if you changed these, you might have broken module-tunnel. > diff --git a/configure.ac b/configure.ac > index 0bf40a8..d57dbc5 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -36,7 +36,7 @@ AC_SUBST(PA_MINOR, pa_minor) > AC_SUBST(PA_MAJORMINOR, pa_major.pa_minor) > > AC_SUBST(PA_API_VERSION, 12) > -AC_SUBST(PA_PROTOCOL_VERSION, 23) > +AC_SUBST(PA_PROTOCOL_VERSION, 24) > > # The stable ABI for client applications, for the version info x:y:z > # always will hold y=z > diff --git a/src/modules/module-tunnel.c b/src/modules/module-tunnel.c > index faee995..0422c87 100644 > --- a/src/modules/module-tunnel.c > +++ b/src/modules/module-tunnel.c > @@ -996,6 +996,41 @@ fail: > pa_module_unload_request(u->module, TRUE); > } > > +static pa_bool_t read_ports(struct userdata *u, pa_tagstruct *t) While in general I agree that a boolean is a fine success/failure return type, I think in Pulseaudio the convention is to stick just to ints. > +{ > + if (u->version >= 16) { > + uint32_t n_ports; > + const char *s; > + > + if (pa_tagstruct_getu32(t, &n_ports)) { > + pa_log("Parse failure"); I would prefer a bit more informative error message ("Parse failure while reading the number of ports."). Oh, now I noticed that this is just old code moving to a new place. Never mind... > + return FALSE; > + } > + > + for (uint32_t j = 0; j < n_ports; j++) { > + uint32_t priority; > + > + if (pa_tagstruct_gets(t, &s) < 0 || /* name */ > + pa_tagstruct_gets(t, &s) < 0 || /* description */ > + pa_tagstruct_getu32(t, &priority) < 0) { > + > + pa_log("Parse failure"); > + return FALSE; > + } > + if (u->version >= 24 && pa_tagstruct_getu32(t, &priority) < 0) { /* available */ > + pa_log("Parse failure"); > + return FALSE; > + } > + } > + > + if (pa_tagstruct_gets(t, &s) < 0) { /* active port */ > + pa_log("Parse failure"); > + return FALSE; > + } > + } > + return TRUE; > +} > + > #ifdef TUNNEL_SINK > > /* Called from main context */ > @@ -1066,32 +1101,8 @@ static void sink_info_cb(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_t > } > } > > - if (u->version >= 16) { > - uint32_t n_ports; > - const char *s; > - > - if (pa_tagstruct_getu32(t, &n_ports)) { > - pa_log("Parse failure"); > - goto fail; > - } > - > - for (uint32_t j = 0; j < n_ports; j++) { > - uint32_t priority; > - > - if (pa_tagstruct_gets(t, &s) < 0 || /* name */ > - pa_tagstruct_gets(t, &s) < 0 || /* description */ > - pa_tagstruct_getu32(t, &priority) < 0) { > - > - pa_log("Parse failure"); > - goto fail; > - } > - } > - > - if (pa_tagstruct_gets(t, &s) < 0) { /* active port */ > - pa_log("Parse failure"); > - goto fail; > - } > - } > + if (!read_ports(u, t)) > + goto fail; > > if (u->version >= 21) { > uint8_t n_formats; > @@ -1318,32 +1329,8 @@ static void source_info_cb(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa > } > } > > - if (u->version >= 16) { > - uint32_t n_ports; > - const char *s; > - > - if (pa_tagstruct_getu32(t, &n_ports)) { > - pa_log("Parse failure"); > - goto fail; > - } > - > - for (uint32_t j = 0; j < n_ports; j++) { > - uint32_t priority; > - > - if (pa_tagstruct_gets(t, &s) < 0 || /* name */ > - pa_tagstruct_gets(t, &s) < 0 || /* description */ > - pa_tagstruct_getu32(t, &priority) < 0) { > - > - pa_log("Parse failure"); > - goto fail; > - } > - } > - > - if (pa_tagstruct_gets(t, &s) < 0) { /* active port */ > - pa_log("Parse failure"); > - goto fail; > - } > - } > + if (!read_ports(u, t)) > + goto fail; > > if (!pa_tagstruct_eof(t)) { > pa_log("Packet too long"); > diff --git a/src/pulse/def.h b/src/pulse/def.h > index f43e864..8a74fad 100644 > --- a/src/pulse/def.h > +++ b/src/pulse/def.h > @@ -967,6 +967,21 @@ typedef void (*pa_free_cb_t)(void *p); > * playback, \since 1.0 */ > #define PA_STREAM_EVENT_FORMAT_LOST "format-lost" > > +/** Port availability / jack detection status > + * \since 2.0 */ > +typedef enum pa_port_available { > + PA_PORT_AVAILABLE_UNKNOWN = 0, /**< This port does not support jack detection \since 2.0 */ > + PA_PORT_AVAILABLE_NO = 1, /**< This port is not available, likely because the jack is not plugged in. \since 2.0 */ > + PA_PORT_AVAILABLE_YES = 2, /**< This port is available, likely because the jack is plugged in. \since 2.0 */ > +} pa_port_available_t; > + > +/** \cond fulldocs */ > +#define PA_PORT_AVAILABLE_UNKNOWN PA_PORT_AVAILABLE_UNKNOWN > +#define PA_PORT_AVAILABLE_NO PA_PORT_AVAILABLE_NO > +#define PA_PORT_AVAILABLE_YES PA_PORT_AVAILABLE_YES > + > +/** \endcond */ > + > PA_C_DECL_END > > #endif > diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c > index 0d1d03f..c8bf7ca 100644 > --- a/src/pulse/introspect.c > +++ b/src/pulse/introspect.c > @@ -211,6 +211,14 @@ static void context_get_sink_info_callback(pa_pdispatch *pd, uint32_t command, u > goto fail; > } > > + i.ports[0][j].available = PA_PORT_AVAILABLE_UNKNOWN; > + if (o->context->version >= 24) { > + uint32_t av; > + if (pa_tagstruct_getu32(t, &av) < 0 || av > PA_PORT_AVAILABLE_YES) > + goto fail; > + i.ports[0][j].available = av; > + } > + > i.ports[j] = &i.ports[0][j]; > } > > @@ -476,6 +484,14 @@ static void context_get_source_info_callback(pa_pdispatch *pd, uint32_t command, > goto fail; > } > > + i.ports[0][j].available = PA_PORT_AVAILABLE_UNKNOWN; > + if (o->context->version >= 24) { > + uint32_t av; > + if (pa_tagstruct_getu32(t, &av) < 0 || av > PA_PORT_AVAILABLE_YES) > + goto fail; > + i.ports[0][j].available = av; > + } > + > i.ports[j] = &i.ports[0][j]; > } > > diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h > index 1d77d45..7732736 100644 > --- a/src/pulse/introspect.h > +++ b/src/pulse/introspect.h > @@ -202,6 +202,7 @@ typedef struct pa_sink_port_info { > const char *name; /**< Name of this port */ > const char *description; /**< Description of this port */ > uint32_t priority; /**< The higher this value is the more useful this port is as a default */ > + pa_port_available_t available; /**< PA_PORT_AVAILABLE_UNKNOWN, PA_PORT_AVAILABLE_NO or PA_PORT_AVAILABLE_YES \since 2.0 */ > } pa_sink_port_info; > > /** Stores information about sinks. Please note that this structure > @@ -281,6 +282,7 @@ typedef struct pa_source_port_info { > const char *name; /**< Name of this port */ > const char *description; /**< Description of this port */ > uint32_t priority; /**< The higher this value is the more useful this port is as a default */ > + pa_port_available_t available; /**< PA_PORT_AVAILABLE_UNKNOWN, PA_PORT_AVAILABLE_NO or PA_PORT_AVAILABLE_YES \since 2.0 */ > } pa_source_port_info; > > /** Stores information about sources. Please note that this structure > diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c > index 0ee4ead..646ecbf 100644 > --- a/src/pulsecore/protocol-native.c > +++ b/src/pulsecore/protocol-native.c > @@ -3102,6 +3102,8 @@ static void sink_fill_tagstruct(pa_native_connection *c, pa_tagstruct *t, pa_sin > pa_tagstruct_puts(t, p->name); > pa_tagstruct_puts(t, p->description); > pa_tagstruct_putu32(t, p->priority); > + if (c->version >= 24) > + pa_tagstruct_putu32(t, p->available); > } > } > > @@ -3173,6 +3175,8 @@ static void source_fill_tagstruct(pa_native_connection *c, pa_tagstruct *t, pa_s > pa_tagstruct_puts(t, p->name); > pa_tagstruct_puts(t, p->description); > pa_tagstruct_putu32(t, p->priority); > + if (c->version >= 24) > + pa_tagstruct_putu32(t, p->available); > } > } > > diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h > index 7f639e2..39ca117 100644 > --- a/src/pulsecore/sink.h > +++ b/src/pulsecore/sink.h > @@ -60,6 +60,7 @@ struct pa_device_port { > char *description; > > unsigned priority; > + pa_port_available_t available; /**< PA_PORT_AVAILABLE_UNKNOWN, PA_PORT_AVAILABLE_NO or PA_PORT_AVAILABLE_YES \since 2.0 */ This is a server-side header - doxygen syntax shouldn't be used, and the \since tag is not needed. I'm not sure the whole comment is very useful, but it does no harm either. > /* .. followed by some implementation specific data */ > }; -- Tanu