[alsa-devel] [PATCH] Sample generation on big endian platforms was broken.
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf Signed-off-by: Kenneth Johansson <kenneth@southpole.se> --- test/pcm.c | 24 ++++++++++++++---------- 1 files changed, 14 insertions(+), 10 deletions(-) diff --git a/test/pcm.c b/test/pcm.c index ee27422..9b8a923 100644 --- a/test/pcm.c +++ b/test/pcm.c @@ -34,14 +34,11 @@ static void generate_sine(const snd_pcm_channel_area_t *areas, static double max_phase = 2. * M_PI; double phase = *_phase; double step = max_phase*freq/(double)rate; - double res; + int res; unsigned char *samples[channels], *tmp; int steps[channels]; - unsigned int chn, byte; - union { - int i; - unsigned char c[4]; - } ires; + unsigned int chn; + unsigned int maxval = (1 << (snd_pcm_format_width(format) - 1)) - 1; int bps = snd_pcm_format_width(format) / 8; /* bytes per sample */ @@ -62,11 +59,18 @@ static void generate_sine(const snd_pcm_channel_area_t *areas, /* fill the channel areas */ while (count-- > 0) { res = sin(phase) * maxval; - ires.i = res; - tmp = ires.c; for (chn = 0; chn < channels; chn++) { - for (byte = 0; byte < (unsigned int)bps; byte++) - *(samples[chn] + byte) = tmp[byte]; + /* Generate data in native endian format */ + switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + } samples[chn] += steps[chn]; } phase += step; -- 1.6.1.GIT
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely... Takashi
--- test/pcm.c | 24 ++++++++++++++---------- 1 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/test/pcm.c b/test/pcm.c index ee27422..9b8a923 100644 --- a/test/pcm.c +++ b/test/pcm.c @@ -34,14 +34,11 @@ static void generate_sine(const snd_pcm_channel_area_t *areas, static double max_phase = 2. * M_PI; double phase = *_phase; double step = max_phase*freq/(double)rate; - double res; + int res; unsigned char *samples[channels], *tmp; int steps[channels]; - unsigned int chn, byte; - union { - int i; - unsigned char c[4]; - } ires; + unsigned int chn; + unsigned int maxval = (1 << (snd_pcm_format_width(format) - 1)) - 1; int bps = snd_pcm_format_width(format) / 8; /* bytes per sample */
@@ -62,11 +59,18 @@ static void generate_sine(const snd_pcm_channel_area_t *areas, /* fill the channel areas */ while (count-- > 0) { res = sin(phase) * maxval; - ires.i = res; - tmp = ires.c; for (chn = 0; chn < channels; chn++) { - for (byte = 0; byte < (unsigned int)bps; byte++) - *(samples[chn] + byte) = tmp[byte]; + /* Generate data in native endian format */ + switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + } samples[chn] += steps[chn]; } phase += step; -- 1.6.1.GIT
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves. It did not understand what problem you see.
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing. See GCC info for details. Takashi
On Fri, 2009-07-03 at 17:00 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing.
See GCC info for details.
Takashi
But there is no aliasing problem in that code. The memory is only accessed(written in this case) using one type.
At Fri, 03 Jul 2009 17:11:37 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:00 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing.
See GCC info for details.
Takashi
But there is no aliasing problem in that code. The memory is only accessed(written in this case) using one type.
One type? + switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + }
At Fri, 03 Jul 2009 17:28:55 +0200, I wrote:
At Fri, 03 Jul 2009 17:11:37 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:00 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing.
See GCC info for details.
Takashi
But there is no aliasing problem in that code. The memory is only accessed(written in this case) using one type.
One type?
+ switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + }
... and the code doesn't handle the case of 3 byte format, too. Takashi
On Fri, 2009-07-03 at 17:41 +0200, Takashi Iwai wrote:
... and the code doesn't handle the case of 3 byte format, too.
Takashi
Soo ? there is a lot of formats this code can't generate. It's a test program and I doubt anybody has ever tried to change the output format since it has never worked for most formats. but if you want to add support for 3 bytes then there is no way around having two different storage methods one for BIG and one for LITTLE endian machines.
At Fri, 03 Jul 2009 19:04:20 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:41 +0200, Takashi Iwai wrote:
... and the code doesn't handle the case of 3 byte format, too.
Takashi
Soo ? there is a lot of formats this code can't generate. It's a test program and I doubt anybody has ever tried to change the output format since it has never worked for most formats.
24bit 3byte format is very popular (e.g. used by USB audio), and the commit that broke the big-endian was to support such formats. It works actually.
but if you want to add support for 3 bytes then there is no way around having two different storage methods one for BIG and one for LITTLE endian machines.
3 byte format is just 24bit linear data packed into 3 bytes. The difference is only the length of the bytes. So, the difference of endian exists. BTW, a simpler solution is to use memcpy() with a certain offset. For example, int offset, size; size = snd_pcm_format_physical_width(fmt) / 8; #ifdef BIG_ENDIAN offset = 4 - size; #else offset = 0; #endif for (...) { ... memcpy(dest, src + offset, size); ... } memcpy() isn't so expensive with the recent gcc/glibc. Could you rewrite the patch in that way and test it on big-endian machines? thanks, Takashi
On Fri, 2009-07-03 at 17:28 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 17:11:37 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:00 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote:
Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf
Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing.
See GCC info for details.
Takashi
But there is no aliasing problem in that code. The memory is only accessed(written in this case) using one type.
One type?
+ switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + }
But only one of them will ever be used. whenever this function is entered the pointer is only using one type ever. the memory is never accessed with any other type and it's only using this one pointer. What exactly do you think the compiler can do ?? it's not like it can cache the samples[chn] value that is the char pointer and make a constant pointer out of it. There is no mixed type accesses. Aliasing happens when two pointers of different type point to the same memory. then you can have funny things happening if you access the memory from the two pointers. There is no pointer aliasing here there is only one pointer. We just change the type of this one pointer in the one place it's used. Is it aliasing with itself ? would the write magically happen in some other size or not at all what ?? could you point me to some documentation for this I'm not getting it.
At Fri, 03 Jul 2009 17:58:50 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:28 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 17:11:37 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 17:00 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:51:35 +0200, Kenneth Johansson wrote:
On Fri, 2009-07-03 at 16:39 +0200, Takashi Iwai wrote:
At Fri, 03 Jul 2009 16:19:31 +0200, Kenneth Johansson wrote: > > Has not worked since commit 3d1fa924906996463ac33cba5b5143f762d913cf > > Signed-off-by: Kenneth Johansson <kenneth@southpole.se>
Hrm, sorry, but your version is also broken as doing type-punning. The code has to be rewritten completely...
Takashi
hmm I think you have to explain this. now it works on both little/big endian without any explicit byte moves.
It did not understand what problem you see.
You can't cast from a char pointer to another type (e.g. short) and read the value. This is called "type-punning" and doesn't work properly with the recent GCC (depending on the optimization and code parsing) since it handles strict-aliasing.
See GCC info for details.
Takashi
But there is no aliasing problem in that code. The memory is only accessed(written in this case) using one type.
One type?
+ switch(bps){ + case 1: + *(samples[chn]) = res; + break; + case 2: + *(short*)(samples[chn]) = res; + break; + case 4: + *(int*)(samples[chn]) = res; + }
But only one of them will ever be used. whenever this function is entered the pointer is only using one type ever. the memory is never accessed with any other type and it's only using this one pointer.
Hrm, OK, that makes sense.
What exactly do you think the compiler can do ?? it's not like it can cache the samples[chn] value that is the char pointer and make a constant pointer out of it. There is no mixed type accesses.
Aliasing happens when two pointers of different type point to the same memory. then you can have funny things happening if you access the memory from the two pointers.
There is no pointer aliasing here there is only one pointer. We just change the type of this one pointer in the one place it's used.
Is it aliasing with itself ? would the write magically happen in some other size or not at all what ??
could you point me to some documentation for this I'm not getting it.
Maybe my unneeded concern. But still your patch breaks the 3-byte format, so it causes another regression. Takashi
participants (2)
-
Kenneth Johansson -
Takashi Iwai