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