[PATCH] Sample generation on big endian platforms was broken.

[PATCH] Sample generation on big endian platforms was broken.

[Kenneth Johansson]

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

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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
> 
> 
> 

Re: [PATCH] Sample generation on big endian platforms was broken.

[Kenneth Johansson]

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.

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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

Re: [PATCH] Sample generation on big endian platforms was broken.

[Kenneth Johansson]

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.

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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;
+			}

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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

Re: [PATCH] Sample generation on big endian platforms was broken.

[Kenneth Johansson]

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.

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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

Re: [PATCH] Sample generation on big endian platforms was broken.

[Kenneth Johansson]

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.

Re: [PATCH] Sample generation on big endian platforms was broken.

[Takashi Iwai]

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