On 10/18/2011 07:16 PM, Tanu Kaskinen wrote:
> Hi,
>
> Looks quite good to me. Some review comments below.
Thanks for the review.
> 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.
Hmm. A quick 'grep -r "return TRUE"' of PulseAudio source tree seems to
give enough results to falsify that assumption.
>> +{
>> + 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.
Ok, I can fix that. Will post new patch version right away.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
