Re: [PATCH v2] msacm32: Rewrite PCM conversion functions.

[Andrew Eikum]

On Tue, Jun 13, 2017 at 04:35:52PM -0500, Zebediah Figura wrote:
> +#define PCM_CVT_CHANGE_FUNC(name)	static void name(DWORD srcRate, const unsigned char *src, DWORD *nsrc, \
> -
> 					 DWORD dstRate, unsigned char *dst, DWORD *ndst) \
> - { \
> - DWORD error = srcRate / 2; \
> - DWORD srcUsed = 0, dstUsed = 0; \
> - for (srcUsed = 0; srcUsed < *nsrc; srcUsed++) { \
> -
>    error += dstRate; \
> -
>    while (error > srcRate) { \
> -
>        if (dstUsed == *ndst) { \
> -
>            *nsrc = srcUsed; \
> -
>            *ndst = dstUsed; \
> -
>            return; \
> -
>        }
> -
> -
>        /* conversion is done here */
> -
>
> +#define PCM_CVT_CHANGE_FUNC_END(offset)	\
> -
>        dstUsed++; \
> -
>        error -= srcRate; \
> -
>    } \
> -
>    src += offset; \
> - } \
> - *nsrc = srcUsed; \
> - *ndst = dstUsed; \
>
> +}
> -
>
> +PCM_CVT_CHANGE_FUNC(cvtSS88C)
> - *dst++ = src[0];
> - *dst++ = src[1];
>
> +PCM_CVT_CHANGE_FUNC_END(2)
> -

I really dislike complicated preprocessor tricks. Can't this be done
in a similar way with real functions? If not, I'd rather have the
copy-pasted code than these difficult macros.

Andrew

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Zebediah Figura]

On 06/14/2017 08:42 AM, Andrew Eikum wrote:
> On Tue, Jun 13, 2017 at 04:35:52PM -0500, Zebediah Figura wrote:
> > +#define PCM_CVT_CHANGE_FUNC(name)	static void name(DWORD srcRate, const unsigned char *src, DWORD *nsrc, \
> > -
> > 					 DWORD dstRate, unsigned char *dst, DWORD *ndst) \
> > - { \
> > - DWORD error = srcRate / 2; \
> > - DWORD srcUsed = 0, dstUsed = 0; \
> > - for (srcUsed = 0; srcUsed < *nsrc; srcUsed++) { \
> > -
> >    error += dstRate; \
> > -
> >    while (error > srcRate) { \
> > -
> >        if (dstUsed == *ndst) { \
> > -
> >            *nsrc = srcUsed; \
> > -
> >            *ndst = dstUsed; \
> > -
> >            return; \
> > -
> >        }
> > -
> > -
> >        /* conversion is done here */
> > -
> >
> > +#define PCM_CVT_CHANGE_FUNC_END(offset)	\
> > -
> >        dstUsed++; \
> > -
> >        error -= srcRate; \
> > -
> >    } \
> > -
> >    src += offset; \
> > - } \
> > - *nsrc = srcUsed; \
> > - *ndst = dstUsed; \
> >
> > +}
> > -
> >
> > +PCM_CVT_CHANGE_FUNC(cvtSS88C)
> > - *dst++ = src[0];
> > - *dst++ = src[1];
> >
> > +PCM_CVT_CHANGE_FUNC_END(2)
> > -
>
> I really dislike complicated preprocessor tricks. Can't this be done
> in a similar way with real functions? If not, I'd rather have the
> copy-pasted code than these difficult macros.
>
> Andrew

As far as I know there's no way to keep this structure without either 
using macros or rewriting the same function 20 times, neither of which 
is very desirable. On the other hand, might it be worth sacrificing a 
slight bit of efficiency (which may not even be that much) as in the 
attached patch?

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Alex Henrie]

2017-06-14 10:13 GMT-06:00 Zebediah Figura :
> On 06/14/2017 08:42 AM, Andrew Eikum wrote:
> > On Tue, Jun 13, 2017 at 04:35:52PM -0500, Zebediah Figura wrote:
> > > +#define PCM_CVT_CHANGE_FUNC(name)      static void name(DWORD srcRate,
> > > const unsigned char *src, DWORD *nsrc, \
> > > -
> > >                                                    DWORD dstRate,
> > >
> > > unsigned char *dst, DWORD *ndst) \
> > > - { \
> > > - DWORD error = srcRate / 2; \
> > > - DWORD srcUsed = 0, dstUsed = 0; \
> > > - for (srcUsed = 0; srcUsed < *nsrc; srcUsed++) { \
> > > -
> > >    error += dstRate; \
> > > -
> > >    while (error > srcRate) { \
> > > -
> > >        if (dstUsed == *ndst) { \
> > > -
> > >            *nsrc = srcUsed; \
> > > -
> > >            *ndst = dstUsed; \
> > > -
> > >            return; \
> > > -
> > >        }
> > > -
> > > -
> > >        /* conversion is done here */
> > > -
> > >
> > > +#define PCM_CVT_CHANGE_FUNC_END(offset)        \
> > > -
> > >        dstUsed++; \
> > > -
> > >        error -= srcRate; \
> > > -
> > >    } \
> > > -
> > >    src += offset; \
> > > - } \
> > > - *nsrc = srcUsed; \
> > > - *ndst = dstUsed; \
> > >
> > > +}
> > > -
> > >
> > > +PCM_CVT_CHANGE_FUNC(cvtSS88C)
> > > - *dst++ = src[0];
> > > - *dst++ = src[1];
> > >
> > > +PCM_CVT_CHANGE_FUNC_END(2)
> > > -
> >
> > I really dislike complicated preprocessor tricks. Can't this be done
> > in a similar way with real functions? If not, I'd rather have the
> > copy-pasted code than these difficult macros.
> >
> > Andrew
>
> As far as I know there's no way to keep this structure without either using
> macros or rewriting the same function 20 times, neither of which is very
> desirable. On the other hand, might it be worth sacrificing a slight bit of
> efficiency (which may not even be that much) as in the attached patch?

You could make each codec function pass a codec-specific callback
function to the common function.

-Alex

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Alex Henrie]

2017-06-14 11:05 GMT-06:00 Alex Henrie :
> 2017-06-14 10:13 GMT-06:00 Zebediah Figura :
> > On 06/14/2017 08:42 AM, Andrew Eikum wrote:
> > > On Tue, Jun 13, 2017 at 04:35:52PM -0500, Zebediah Figura wrote:
> > > > +#define PCM_CVT_CHANGE_FUNC(name)      static void name(DWORD srcRate,
> > > > const unsigned char *src, DWORD *nsrc, \
> > > > -
> > > >                                                    DWORD dstRate,
> > > >
> > > > unsigned char *dst, DWORD *ndst) \
> > > > - { \
> > > > - DWORD error = srcRate / 2; \
> > > > - DWORD srcUsed = 0, dstUsed = 0; \
> > > > - for (srcUsed = 0; srcUsed < *nsrc; srcUsed++) { \
> > > > -
> > > >    error += dstRate; \
> > > > -
> > > >    while (error > srcRate) { \
> > > > -
> > > >        if (dstUsed == *ndst) { \
> > > > -
> > > >            *nsrc = srcUsed; \
> > > > -
> > > >            *ndst = dstUsed; \
> > > > -
> > > >            return; \
> > > > -
> > > >        }
> > > > -
> > > > -
> > > >        /* conversion is done here */
> > > > -
> > > >
> > > > +#define PCM_CVT_CHANGE_FUNC_END(offset)        \
> > > > -
> > > >        dstUsed++; \
> > > > -
> > > >        error -= srcRate; \
> > > > -
> > > >    } \
> > > > -
> > > >    src += offset; \
> > > > - } \
> > > > - *nsrc = srcUsed; \
> > > > - *ndst = dstUsed; \
> > > >
> > > > +}
> > > > -
> > > >
> > > > +PCM_CVT_CHANGE_FUNC(cvtSS88C)
> > > > - *dst++ = src[0];
> > > > - *dst++ = src[1];
> > > >
> > > > +PCM_CVT_CHANGE_FUNC_END(2)
> > > > -
> > >
> > > I really dislike complicated preprocessor tricks. Can't this be done
> > > in a similar way with real functions? If not, I'd rather have the
> > > copy-pasted code than these difficult macros.
> > >
> > > Andrew
> >
> > As far as I know there's no way to keep this structure without either using
> > macros or rewriting the same function 20 times, neither of which is very
> > desirable. On the other hand, might it be worth sacrificing a slight bit of
> > efficiency (which may not even be that much) as in the attached patch?
>
> You could make each codec function pass a codec-specific callback
> function to the common function.

Actually, forget I said anything. The patch you attached is similar,
but even cleaner than what I was thinking.

-Alex

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Zebediah Figura]

On 06/14/2017 12:13 PM, Alex Henrie wrote:
> 2017-06-14 11:05 GMT-06:00 Alex Henrie :
> > 2017-06-14 10:13 GMT-06:00 Zebediah Figura :
> > > On 06/14/2017 08:42 AM, Andrew Eikum wrote:
> > > > On Tue, Jun 13, 2017 at 04:35:52PM -0500, Zebediah Figura wrote:
> > > > > +#define PCM_CVT_CHANGE_FUNC(name)      static void name(DWORD srcRate,
> > > > > const unsigned char *src, DWORD *nsrc, \
> > > > > -
> > > > >                                                    DWORD dstRate,
> > > > >
> > > > > unsigned char *dst, DWORD *ndst) \
> > > > > - { \
> > > > > - DWORD error = srcRate / 2; \
> > > > > - DWORD srcUsed = 0, dstUsed = 0; \
> > > > > - for (srcUsed = 0; srcUsed < *nsrc; srcUsed++) { \
> > > > > -
> > > > >    error += dstRate; \
> > > > > -
> > > > >    while (error > srcRate) { \
> > > > > -
> > > > >        if (dstUsed == *ndst) { \
> > > > > -
> > > > >            *nsrc = srcUsed; \
> > > > > -
> > > > >            *ndst = dstUsed; \
> > > > > -
> > > > >            return; \
> > > > > -
> > > > >        }
> > > > > -
> > > > > -
> > > > >        /* conversion is done here */
> > > > > -
> > > > >
> > > > > +#define PCM_CVT_CHANGE_FUNC_END(offset)        \
> > > > > -
> > > > >        dstUsed++; \
> > > > > -
> > > > >        error -= srcRate; \
> > > > > -
> > > > >    } \
> > > > > -
> > > > >    src += offset; \
> > > > > - } \
> > > > > - *nsrc = srcUsed; \
> > > > > - *ndst = dstUsed; \
> > > > >
> > > > > +}
> > > > > -
> > > > >
> > > > > +PCM_CVT_CHANGE_FUNC(cvtSS88C)
> > > > > - *dst++ = src[0];
> > > > > - *dst++ = src[1];
> > > > >
> > > > > +PCM_CVT_CHANGE_FUNC_END(2)
> > > > > -
> > > >
> > > > I really dislike complicated preprocessor tricks. Can't this be done
> > > > in a similar way with real functions? If not, I'd rather have the
> > > > copy-pasted code than these difficult macros.
> > > >
> > > > Andrew
> > >
> > > As far as I know there's no way to keep this structure without either using
> > > macros or rewriting the same function 20 times, neither of which is very
> > > desirable. On the other hand, might it be worth sacrificing a slight bit of
> > > efficiency (which may not even be that much) as in the attached patch?
> >
> > You could make each codec function pass a codec-specific callback
> > function to the common function.
>
> Actually, forget I said anything. The patch you attached is similar,
> but even cleaner than what I was thinking.
>
> -Alex

Yeah, the idea of the original is presumably to avoid having to do a 
jump/call on every loop iteration.

I wrote a test program to convert blocks of 100 MB, and the original is 
over twice as fast (converting 100 blocks of 100 MB each took 17.972s 
with this patch, 39.114s with my updated patch.) That's a pretty 
significant difference, although it's presumably less noticeable when 
the size is smaller.

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Andrew Eikum]

On Wed, Jun 14, 2017 at 11:13:21AM -0500, Zebediah Figura wrote:
> On 06/14/2017 08:42 AM, Andrew Eikum wrote:
> > I really dislike complicated preprocessor tricks. Can't this be done
> > in a similar way with real functions? If not, I'd rather have the
> > copy-pasted code than these difficult macros.
> >
> > Andrew
>
> As far as I know there's no way to keep this structure without either using
> macros or rewriting the same function 20 times, neither of which is very
> desirable. On the other hand, might it be worth sacrificing a slight bit of
> efficiency (which may not even be that much) as in the attached patch?

I haven't totally reviewed this yet, but yeah, I like this far better.

Andrew

[PATCH v2] msacm32: Rewrite PCM conversion functions.

[Alexandre Julliard]

Zebediah Figura  writes:
> As far as I know there's no way to keep this structure without either
> using macros or rewriting the same function 20 times, neither of which
> is very desirable. On the other hand, might it be worth sacrificing a
> slight bit of efficiency (which may not even be that much) as in the
> attached patch?

You'd probably want to measure it, but I expect that one function call
per sample is going to be a lot more than a slight inefficiency. That
may or may not matter in actual usage of course.