[Bug 773073] audioconvert: endian conversion optimization

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=773073

--- Comment #42 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
(In reply to Petr Kulhavy from comment #41)
> (In reply to Sebastian Dröge (slomo) from comment #40)
> > Review of attachment 338444 [details] [review] [review]:
> > 
> > Generally looks good, thanks :) Just some minor remarks below
> > 
> > ::: gst-libs/gst/audio/audio-converter.c
> > @@ +863,3 @@
> > + */
> > +static void
> > +converter_swap_endian_24 (gpointer dst, const gpointer src, gint count)
> > 
> > Worries me a bit that dst and src can alias (in place transform), a compiler
> > might optimize the code below to something broken in theory: it could get
> > "x" after writing to in[i] for optimization reasons under the assumption
> > that they don't alias
> > 
> > Maybe we need a version for in-place and not?
> 
> That's a good point. I thought that introducing the 'x' would solve it but 
> haven't thought of it being optimized out, there you're absolutely right.
> Would the XOR algorithm solve it?
> [...]

Maybe but that's really awful :)

> And isn't this also an issue for the ORC versions?

Yes, ORC explicitly requires that there is no aliasing in the function
arguments (it even uses the C99 restrict keyword).

> I'm also thinking that the function header (src being const) might be
> formally wrong because it is called with src==dst.
> 
> I'm trying to avoid another function if possible because it makes either the
> code less legible and more complex (another function pointer, all functions
> doubled, and another test in convert_endian() ) or inefficient (if the test
> is done inside the swap_endian() itself).

Could generate those duplicated functions with a macro, I think that's my
preferred solution right now. Let's do any fixes for this as a commit on top of
the existing one though.

> > @@ +1080,3 @@
> > +            ("same formats, no resampler and passthrough mixing ->
> > passthrough");
> > +        convert->convert = converter_passthrough;
> > +        /* TODO: implement also in-place conversion */
> > 
> > Isn't this (converter_passthrough) never called anyway as basetransform will
> > work in passthrough mode then? And converter_passthrough could just check
> > for in==out and return then
> 
> This code has already been already there I haven't touched it. But in my
> tests I haven't seen the converter_passthrough to be called at all. This
> might be due to "basetransform_class->passthrough_on_same_caps = TRUE;"
> being set in gst_audio_convert_class_init() But on the other hand that is
> outside of this class... So formally the converter_passthrough is IMHO
> correct. It just makes no sense ;-)

Well, something else might use the API and call it :) I guess that TODO comment
just makes no sense.

> > ::: gst/audioconvert/gstaudioconvert.c
> > @@ +791,3 @@
> > +gst_audio_convert_transform_ip (GstBaseTransform * base, GstBuffer * buf)
> > +{
> > +  return gst_audio_convert_transform (base, buf, buf);
> > 
> > Maybe needs a g_assert() for gst_audio_converter_supports_inplace() to
> > return TRUE :)
> 
> Isn't this one in _transform() enough?
> 
>   /* allow inbuf==outbuf (i.e. entry from transform_ip)
>    * only if the buffer is writable */
>   g_assert (inbuf != outbuf || inbuf_writable);

Yes, but it would be more explicit and understandable to have (also) this
assertion in transform_ip()

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.