[Bug 773073] audioconvert: endian conversion optimization

[GStreamer (GNOME Bugzilla)]

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

--- Comment #41 from Petr Kulhavy <brain at jikos.cz> ---
(In reply to Sebastian Dröge (slomo) from comment #40)
> Review of attachment 338444 [details] [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?

  for (i = 0; i < count; i += 3) {
    guint8 a = in[i + 0];
    guint8 c = in[i + 2];

    a ^= c;
    c ^= a;
    a ^= c;

    out[i + 0] = a;
    out[i + 1] = in[i + 1];
    out[i + 2] = c;
  }

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

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

> @@ +1002,3 @@
>  }
>  
> +#define GST_AUDIO_FORMAT_IS_ENDIAN_CONVERSION(info1, info2) \
> 
> And next step, sign conversion \o/ :)

Yes, but let's finish this one first, 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 ;-)

> @@ +1116,3 @@
> +          default:
> +            GST_ERROR ("unsupported sample width for endian conversion");
> +            g_assert (0);
> 
> g_assert_not_reached()

Thanks.

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

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