[Bug 773073] audioconvert: endian conversion optimization

[GStreamer (GNOME Bugzilla)]

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

--- Comment #44 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
(In reply to Petr Kulhavy from comment #43)

> > > 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).
> 
> Hmmm, that moves us slightly backwards... What is then the way to do ORC
> endian swap in place?

Have an orc function with only one argument, the destination array.

> > > > @@ +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.
> 
> The point is that the introduction of transform_ip() semantically slightly
> changes the API of audio-converter. So far the conversion has never been
> thought to be in place (even though it wasn't very clear), but now by
> implementing transform_ip() we are de-facto allowing it. So if for whatever
> reason someone decides to call convert() in passthrough (just because they
> can) it would be logical and consistent that it indicates in-place=TRUE and
> convert() doesn't do anything if it detects src==dest.
> 
> Effectively it's a duplication of what's been already done in
> base-transform, but that's the price for having audioconvert as a public
> class.

Yes, also trivial to handle that correctly so lets just do that :)

> > > > ::: 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()
> 
> Agree, but probably also slower as it is an additional function call.

It's a macro, almost no cost here



For the aliasing, actually things are better than expected. There's only a
problem with ORC here because it uses "restrict" for the arrays (which hints at
the compiler that they will not alias). For the normal C code there is no
problem as the compiler is only allowed to assume no aliasing happens if there
is "restrict", or there are two pointers of a different type (which is not the
case here).

I would suggest that for simplicity we get rid of the ORC parts here and just
do it all in C. Then we only need one function and not a separate one for
in-place. And the speed difference is probably not big (right?).

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