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

[David Henningsson]

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 
protocol-native.c).

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.
https://launchpad.net/~diwic