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.
> Should we put an "inline" hint on extend?
Well, my approach is to never use "inline" outside headers, because I
trust the compiler to know better than me when inlining makes sense. But
I'm not against adding "inline" to extend(). It's only a hint after all,
the compiler is still free to do whatever it wants. Maybe it would be
good to drop those assertions too, if performance is a concern?
> 2) Peter also has some tagstruct performance stuff in the pipeline,
> will your patches cause a merge conflict?
I'm not familiar with Peter's patches, but since my patch is so
invasive, conflicts seem very likely.
I'm not in any particular hurry with this patch. If you prefer to get
Peter's patches in first, and then rebase this patch, I'm ok with that.
> 3) To what degree have you tested this?
I've tested this when I've tested the work-in-progress volume control
patches. "pactl list" probably exercises pretty much all of this code,
and it's working fine.
> > +static int check_type(pa_tagstruct *t, uint8_t type) {
>
> I think this should be renamed to "read_tag" to indicate that it
> advances the read pointer.
Ok, makes sense.
> > int pa_tagstruct_get_usec(pa_tagstruct*t, pa_usec_t *u) {
> > - uint32_t tmp;
> > -
> > pa_assert(t);
> > pa_assert(u);
> >
> > - if (t->rindex+9 > t->length)
> > + if (check_type(t, PA_TAG_USEC) < 0)
> > return -1;
> >
> > - if (t->data[t->rindex] != PA_TAG_USEC)
> > + if (read_u64(t, u) < 0)
> > return -1;
>
> You can shorten things even further:
>
> return read_tag(t, PA_TAG_USEC) < 0 ? read_u64(t, u) : -1;
>
> 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.
--
Tanu
