[Bug 773073] audioconvert: endian conversion optimization

[GStreamer (GNOME Bugzilla)]

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

--- Comment #43 from Petr Kulhavy <brain at jikos.cz> ---
(In reply to Sebastian Dröge (slomo) from comment #42)

> > 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 :)

Well, I call this smart and elegant ;-)

> > 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?

> > 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.

Not sure if I understand what you mean. Could you give an example, please.

> > > @@ +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.

> > > ::: 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.

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