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
