[pulseaudio-discuss] [PATCH] tagstruct: Refactor writing/reading basic types

David Henningsson david.henningsson at canonical.com
Mon Feb 16 22:40:40 PST 2015

On 2015-02-16 20:27, Tanu Kaskinen wrote:
> On Mon, 2015-02-16 at 12:28 +0100, David Henningsson wrote:
>> On 2015-01-07 20:51, Tanu Kaskinen wrote:
>>> While adding functions for writing and reading pa_bvolume structs, I
>>> found myself wondering if I could make it simpler to write and read
>>> the basic types that a pa_bvolume consists of, without having to worry
>>> about network byte ordering, remembering to call extend() and getting
>>> the length and read index adjustments just right. This is what I came
>>> up with.
>> A lot of things to read here :-)
>> In general this seems to be a nice ergonomic improvement and I couldn't
>> find any bugs.
>> First some general thoughts:
>>    1) This means calling "extend" more times than before, for combined
>> stuff. Is this causing worse performance?
> Probably yes. I haven't measured. I'd guess the performance degradation
> is negligible, though, since (AFAIK) tagstructs are not used in audio
> streaming. I don't think there's any use case where there would be so
> many tagstructs flowing around that a few extra function calls would
> cause significant slowdown.

For every playback packet, there are three packets sent over the wire:
  * data memblock (client -> server)
  * data memblock release (server -> client)
  * request more data (server -> client)

The third one is a tagstruct (look for PA_COMMAND_REQUEST in 

In addition, there are latency updates which many clients request every 
now and then.

My gut feeling is that the potential performance hit isn't noticeable, 
but I'm not sure.

>> You can shorten things even further:
>>       return read_tag(t, PA_TAG_USEC) < 0 ? read_u64(t, u) : -1;

Should have been:

    return read_tag(t, PA_TAG_USEC) < 0 ? -1 : read_u64(t, u)

>> Or if you think that's ugly:
>>       if (read_tag(t, PA_TAG_USEC) < 0)
>>           return -1;
>>       return read_u64(t, u);
>> (This might apply to more places)
> Ok, I'll use the second suggestion, and see where else it might apply.
>>>    int pa_tagstruct_get_proplist(pa_tagstruct *t, pa_proplist *p) {
>>> -    size_t saved_rindex;
>> The saved_rindex seems to be here for a reason. If you now remove it,
>> have you checked all call sites that they don't make use of it?
>> I'm thinking maybe it tries reading some other type if it can't find a
>> proplist, or something like that.
> Yes, I checked. I didn't find any code that would react to a failure by
> trying to read something else instead.
> It would be nice if the pa_tagstruct_get_foo() functions wouldn't modify
> the tagstruct state on failure, but I didn't figure out a nice way to
> retain that property. I think it's reasonable to assume that if errors
> are detected while reading a tagstruct, the tagstruct contents are
> ignored altogether.

Ok, then that's fine. It would have been good to either have this in a 
separate patch or at least a line in the commit message about it. Since 
it is actually not plain refactoring but a change of behaviour as well.

David Henningsson, Canonical Ltd.

More information about the pulseaudio-discuss mailing list