Re: [PATCH v2] msacm32: Rewrite PCM conversion functions.
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
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?
2017-06-14 10:13 GMT-06:00 Zebediah Figura <z.figura12(a)gmail.com>:
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
2017-06-14 11:05 GMT-06:00 Alex Henrie <alexhenrie24(a)gmail.com>:
2017-06-14 10:13 GMT-06:00 Zebediah Figura <z.figura12(a)gmail.com>:
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
On 06/14/2017 12:13 PM, Alex Henrie wrote:
2017-06-14 11:05 GMT-06:00 Alex Henrie <alexhenrie24(a)gmail.com>:
2017-06-14 10:13 GMT-06:00 Zebediah Figura <z.figura12(a)gmail.com>:
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.
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
Zebediah Figura <z.figura12(a)gmail.com> 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. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alex Henrie -
Alexandre Julliard -
Andrew Eikum -
Zebediah Figura