Re: [pulseaudio-discuss] [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels
We have got three confirmations that the patch is working in the bug below, so I believe it can be safely applied to both master and stable-queue. On 2010-10-08 19:00, David Henningsson wrote: I would kindly ask for comments for this patch before applying, just double-check that I thought the right way when fixing this one. BugLink: https://launchpad.net/bugs/445849 In the assembly optimized versions of SSE, a noise could occur when the number of channels were 3,5,6 or 7. For MMX and ARM, this could occur when the number of channels were 3. Signed-off-by: David Henningsson david.hennings...@canonical.com ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels
'Twas brillig, and David Henningsson at 13/10/10 07:27 did gyre and gimble: We have got three confirmations that the patch is working in the bug below, so I believe it can be safely applied to both master and stable-queue. Great! Would you mind respinning the patch with a couple more inline comments? I'm a little concerned about the lines such as channels = channels == 3 ? 6 : PA_MAX() It will be a bit disconcerting to read this in code without really reading the relevant commit message, so I think a small hint towards why this is done would be good here. Or perhaps create a couple macros instead then document the macro definitions? Whatever you prefer, but I think some hints are needed at the code level. Cheers Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mageia Contributor [http://www.mageia.org/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels
On 2010-10-13 09:53, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 13/10/10 07:27 did gyre and gimble: We have got three confirmations that the patch is working in the bug below, so I believe it can be safely applied to both master and stable-queue. Great! Would you mind respinning the patch with a couple more inline comments? I'm a little concerned about the lines such as channels = channels == 3 ? 6 : PA_MAX() It will be a bit disconcerting to read this in code without really reading the relevant commit message, so I think a small hint towards why this is done would be good here. Thanks for reviewing the patch, I agree that a little clearer comment wouldn't hurt, so here's a revised version. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic From 0bde1e26fd9b2b6c1cf3d914ad11347b0d852539 Mon Sep 17 00:00:00 2001 From: David Henningsson david.hennings...@canonical.com Date: Fri, 8 Oct 2010 18:47:00 +0200 Subject: [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels In the assembly optimized versions of SSE, a noise could occur when the number of channels were 3,5,6 or 7. For MMX and ARM, this could occur when the number of channels were 3. Signed-off-by: David Henningsson david.hennings...@canonical.com --- src/pulsecore/svolume_arm.c |5 - src/pulsecore/svolume_mmx.c | 14 -- src/pulsecore/svolume_sse.c | 19 +-- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/pulsecore/svolume_arm.c b/src/pulsecore/svolume_arm.c index 5bd1448..fdd8f09 100644 --- a/src/pulsecore/svolume_arm.c +++ b/src/pulsecore/svolume_arm.c @@ -47,7 +47,10 @@ pa_volume_s16ne_arm (int16_t *samples, int32_t *volumes, unsigned channels, unsi { int32_t *ve; -channels = PA_MAX (4U, channels); +/* Channels must be at least 4, and always a multiple of the original number. + * This is also the max amount we overread the volume array, which should + * have enough padding. */ +channels = channels == 3 ? 6 : PA_MAX (4U, channels); ve = volumes + channels; __asm__ __volatile__ ( diff --git a/src/pulsecore/svolume_mmx.c b/src/pulsecore/svolume_mmx.c index e50ebee..a71a39b 100644 --- a/src/pulsecore/svolume_mmx.c +++ b/src/pulsecore/svolume_mmx.c @@ -98,9 +98,10 @@ pa_volume_s16ne_mmx (int16_t *samples, int32_t *volumes, unsigned channels, unsi { pa_reg_x86 channel, temp; -/* the max number of samples we process at a time, this is also the max amount - * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (4U, channels); +/* Channels must be at least 4, and always a multiple of the original number. + * This is also the max amount we overread the volume array, which should + * have enough padding. */ +channels = channels == 3 ? 6 : PA_MAX (4U, channels); __asm__ __volatile__ ( xor %3, %3\n\t @@ -164,9 +165,10 @@ pa_volume_s16re_mmx (int16_t *samples, int32_t *volumes, unsigned channels, unsi { pa_reg_x86 channel, temp; -/* the max number of samples we process at a time, this is also the max amount - * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (4U, channels); +/* Channels must be at least 4, and always a multiple of the original number. + * This is also the max amount we overread the volume array, which should + * have enough padding. */ +channels = channels == 3 ? 6 : PA_MAX (4U, channels); __asm__ __volatile__ ( xor %3, %3\n\t diff --git a/src/pulsecore/svolume_sse.c b/src/pulsecore/svolume_sse.c index 1cc4e0a..5983164 100644 --- a/src/pulsecore/svolume_sse.c +++ b/src/pulsecore/svolume_sse.c @@ -74,14 +74,19 @@ por %%xmm4, #s1\n\t /* .. | l h | */ \ por %%xmm5, #s2\n\t + +static int channel_overread_table[8] = {8,8,8,12,8,10,12,14}; + static void pa_volume_s16ne_sse2 (int16_t *samples, int32_t *volumes, unsigned channels, unsigned length) { pa_reg_x86 channel, temp; -/* the max number of samples we process at a time, this is also the max amount - * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (8U, channels); +/* Channels must be at least 8 and always a multiple of the original number. + * This is also the max amount we overread the volume array, which should + * have enough padding. */ +if (channels 8) +channels = channel_overread_table[channels]; __asm__ __volatile__ ( xor %3, %3\n\t @@ -159,9 +164,11 @@ pa_volume_s16re_sse2 (int16_t *samples, int32_t *volumes, unsigned channels, uns { pa_reg_x86 channel, temp; -/* the max number of samples we process at a time, this is also the max amount - * we overread the volume array, which should have enough padding. */ -channels =
Re: [pulseaudio-discuss] [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels
'Twas brillig, and David Henningsson at 13/10/10 10:18 did gyre and gimble: On 2010-10-13 09:53, Colin Guthrie wrote: 'Twas brillig, and David Henningsson at 13/10/10 07:27 did gyre and gimble: We have got three confirmations that the patch is working in the bug below, so I believe it can be safely applied to both master and stable-queue. Great! Would you mind respinning the patch with a couple more inline comments? I'm a little concerned about the lines such as channels = channels == 3 ? 6 : PA_MAX() It will be a bit disconcerting to read this in code without really reading the relevant commit message, so I think a small hint towards why this is done would be good here. Thanks for reviewing the patch, I agree that a little clearer comment wouldn't hurt, so here's a revised version. Great, thanks. Pushed now to master and s-q. Col -- Colin Guthrie gmane(at)colin.guthr.ie http://colin.guthr.ie/ Day Job: Tribalogic Limited [http://www.tribalogic.net/] Open Source: Mageia Contributor [http://www.mageia.org/] PulseAudio Hacker [http://www.pulseaudio.org/] Trac Hacker [http://trac.edgewall.org/] ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels
I would kindly ask for comments for this patch before applying, just double-check that I thought the right way when fixing this one. BugLink: https://launchpad.net/bugs/445849 In the assembly optimized versions of SSE, a noise could occur when the number of channels were 3,5,6 or 7. For MMX and ARM, this could occur when the number of channels were 3. Signed-off-by: David Henningsson david.hennings...@canonical.com -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic From a7a099463370c1f49e470597fb7a70a6ac72d685 Mon Sep 17 00:00:00 2001 From: David Henningsson david.hennings...@canonical.com Date: Fri, 8 Oct 2010 18:47:00 +0200 Subject: [PATCH] SSE/MMX/ARM: Fix high frequency noise with unusual number of channels BugLink: https://launchpad.net/bugs/445849 In the assembly optimized versions of SSE, a noise could occur when the number of channels were 3,5,6 or 7. For MMX and ARM, this could occur when the number of channels were 3. Signed-off-by: David Henningsson david.hennings...@canonical.com --- src/pulsecore/svolume_arm.c |2 +- src/pulsecore/svolume_mmx.c |4 ++-- src/pulsecore/svolume_sse.c |9 +++-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/pulsecore/svolume_arm.c b/src/pulsecore/svolume_arm.c index 5bd1448..ec9c08a 100644 --- a/src/pulsecore/svolume_arm.c +++ b/src/pulsecore/svolume_arm.c @@ -47,7 +47,7 @@ pa_volume_s16ne_arm (int16_t *samples, int32_t *volumes, unsigned channels, unsi { int32_t *ve; -channels = PA_MAX (4U, channels); +channels = channels == 3 ? 6 : PA_MAX (4U, channels); ve = volumes + channels; __asm__ __volatile__ ( diff --git a/src/pulsecore/svolume_mmx.c b/src/pulsecore/svolume_mmx.c index e50ebee..5141d6c 100644 --- a/src/pulsecore/svolume_mmx.c +++ b/src/pulsecore/svolume_mmx.c @@ -100,7 +100,7 @@ pa_volume_s16ne_mmx (int16_t *samples, int32_t *volumes, unsigned channels, unsi /* the max number of samples we process at a time, this is also the max amount * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (4U, channels); +channels = channels == 3 ? 6 : PA_MAX (4U, channels); __asm__ __volatile__ ( xor %3, %3\n\t @@ -166,7 +166,7 @@ pa_volume_s16re_mmx (int16_t *samples, int32_t *volumes, unsigned channels, unsi /* the max number of samples we process at a time, this is also the max amount * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (4U, channels); +channels = channels == 3 ? 6 : PA_MAX (4U, channels); __asm__ __volatile__ ( xor %3, %3\n\t diff --git a/src/pulsecore/svolume_sse.c b/src/pulsecore/svolume_sse.c index 1cc4e0a..afba877 100644 --- a/src/pulsecore/svolume_sse.c +++ b/src/pulsecore/svolume_sse.c @@ -74,6 +74,9 @@ por %%xmm4, #s1\n\t /* .. | l h | */ \ por %%xmm5, #s2\n\t + +static int channel_overread_table[8] = {8,8,8,12,8,10,12,14}; + static void pa_volume_s16ne_sse2 (int16_t *samples, int32_t *volumes, unsigned channels, unsigned length) { @@ -81,7 +84,8 @@ pa_volume_s16ne_sse2 (int16_t *samples, int32_t *volumes, unsigned channels, uns /* the max number of samples we process at a time, this is also the max amount * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (8U, channels); +if (channels 8) +channels = channel_overread_table[channels]; __asm__ __volatile__ ( xor %3, %3\n\t @@ -161,7 +165,8 @@ pa_volume_s16re_sse2 (int16_t *samples, int32_t *volumes, unsigned channels, uns /* the max number of samples we process at a time, this is also the max amount * we overread the volume array, which should have enough padding. */ -channels = PA_MAX (8U, channels); +if (channels 8) +channels = channel_overread_table[channels]; __asm__ __volatile__ ( xor %3, %3\n\t -- 1.7.1 ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss