[pulseaudio-discuss] [PATCH] Introduce "available" concept for ports, and communicate that to clients. Bump protocol version to 24.

[David Henningsson]

Thanks for the review.

On 10/20/2011 10:25 AM, Arun Raghavan wrote:
> 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>
>> ---
> [...]
>> 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
>> +
>
> Is there a reason for this to be larger than uint8_t?

Not at this point, but diverging from Ubuntu 11.10's implementation 
would not resolve the protocol skew.

>>   #### 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.
>
> Does module-tunnel need to care about any of this?

Yes, did you miss that part of the patch?

> [...]
>> [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
>
> I'd drop the "likely because" since that's going to depend on other bits
> of implementation (for example a jack maybe unavailable on the current
> profile

The port available status is independent from whether a profile is 
active or not, and also, a port can belong to several profiles (in 
patches soon to come).

> and your comment will only be true if there's a policy module to
> switch profiles on jack insertion).

No, it will be true if there is a port implementation that sets jack 
availability status.

>
>> 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;
>
> I think the docs already link to the enum type, so the long comment
> might be replaced by "Whether the port is available or not" or something
> similar.

Ok, want me to resend with the comment changed?

Also, for the bool vs int thing, I think we all agree on that bools are 
okay in this context.

>
> [...]
>> 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 */
>>
>>       /* .. followed by some implementation specific data */
>>   };
>
> I think someone already mentioned this doesn't need to be a doxygen
> comment.

This is already fixed in latest posted version of patch (which, for 
reference, is here 
http://lists.freedesktop.org/archives/pulseaudio-discuss/2011-October/011833.html 
)


-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic