[pulseaudio-discuss] [PATCH 0/2] module-tunnel-source broken - Something went wrong at protocol version 22 :-(

[David Henningsson]

On 02/07/2012 11:50 AM, Arun Raghavan wrote:
> On Mon, 2012-01-30 at 16:47 +0100, David Henningsson wrote:
>> I'm trying to set up a module-tunnel-source, but it fails with a
>> protocol error.
>>
>> Here's the analysis.
>>
>> According to spec:
>>
>> <---
>> ## v22, implemented by>= 1.0
>>
>> New fields PA_COMMAND_CREATE_RECORD_STREAM:
>>
>>       uint8_t n_formats
>>       format_info format1
>>       ...
>>       format_info formatn
>> --->
>>
>>
>> 1) According to code in module-tunnel.c, the code for sending n_formats
>> is missing.
>>
>> 2) More interesting is that according to code in protocol-native.c, not
>> only does PA_COMMAND_CREATE_RECORD_STREAM assume the above to come in,
>> but also these fields:
>>
>> <---
>>           if (pa_tagstruct_get_cvolume(t,&volume)<  0 ||
>>               pa_tagstruct_get_boolean(t,&muted)<  0 ||
>>               pa_tagstruct_get_boolean(t,&volume_set)<  0 ||
>>               pa_tagstruct_get_boolean(t,&muted_set)<  0 ||
>>               pa_tagstruct_get_boolean(t,&relative_volume)<  0 ||
>>               pa_tagstruct_get_boolean(t,&passthrough)<  0) {
>> --->
>>
>> ...which is consistent with the code in src/pulse/stream.c. These are
>> not documented in the PROTOCOL file.
>>
>> 3) Actually, they remotely correspond to something in the documentation
>> for protocol v22 as well:
>>
>> <---
>> Five new fields in reply from PA_COMMAND_GET_SOURCE_OUTPUT_INFO (and
>> thus PA_COMMAND_GET_SOURCE_OUTPUT_INFO_LIST)
>>
>>       format_info format
>>       volume
>>       bool mute
>>       bool has_volume
>>       bool volume_writable
>> --->
>>
>> In the code, the format_info comes last instead of first. :-/
>>
>> This is a problem both in PulseAudio 1.x and in git master. (And thus in
>> Ubuntu 11.10 and Ubuntu 12.04.)
>>
>> After some thoughts, I think the wisest course of action is to fixup
>> module-tunnel to send more fields, and fixup the documentation to match
>> the code. I'll send some patches for this. Next question is if we should
>> also consider backwards compatibility with 1.x's broken implementation
>> of module-tunnel-source...?
>
> Sigh. We really need to automate testing this - it breaks too often. I
> don't think we should aim for backward compatibility with the incorrect
> implementation. IMO we could either wait for the next release or do a
> quick 1.2 and ask people complaining about this to upgrade. Yes, this
> sucks, but I'd rather do that than inflict backwards-compatibility
> kludge.
>
> If there are no objections, I'll pull the patches.

I'm okay with not being backwards compatible as well. It does not seem 
like a widely used feature though, as it took this long to discover the bug.

Here are a few ideas:

1) For testing, we could do something like testing connections on all 
protocol versions from 1 up to the current one.

2) We could consider having separate protocol versions for module-tunnel 
and the rest in configure.ac. This makes it both more visible, and 
enables the possibility for the tunnel protocol to lag behind.

3) And the most radical one: Why is module-tunnel done the way it is? 
Why couldn't we instead use PulseAudio's client API on the server side? 
Then we don't need to maintain two client implementations of the 
protocol in the first place!

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