Re: Bug#289182: kino endianness issues on powerpc

[Daniel Kobras <kobras@debian.org>]

On Tue, Mar 22, 2005 at 08:36:01PM +0000, Paul Brossier wrote:
> On Tue, Mar 08, 2005 at 03:39:16PM +0100, Daniel Kobras wrote:
> > Furthermore, it looks obviously buggy. Eg. the little-endian
> > version of the first loop uses values Y[0] and Y[1], while the
> > big-endian variant reuses Y[0] twice. And I can't make sense of
> > the other array indices, either. I was expecting something like
> > dest_big_endian = bswap_32(dest_little_endian); maybe that's
> > what was intended, and the current version of the patch makes
> > little difference with smooth input data? 
> 
> well, i am not familiar with these maths, but it does look like
> there's some logic:
> 
>         LE           -> BE

> 
>         Cr[0] << 24  -> Cr[0]
>         Y [1] << 16  -> Y [0] << 8
>         Cb[0] << 8   -> Cb[1] << 16
>         Y [0]        -> Y [0] << 24

Certainly, but as I noted before, this logic is flawed, incorrectly
reusing an old luminance value and shifting the blue component by one
pixel. (In a smooth image, artifacts might be small, though.) 

> there is probably a better mean to do so though, ie checking what
> type of conversion is needed according to libavcodec, but it does
> effectively fixes the XV Display of kino.

Actually, we should probably check the value of component_order[] in the
struct returned by XvListImageFormats() and shuffle components as
appropriate, but from the feedback so far, apparently nowadays it is
sufficient to keep the order of YUV components fixed, independent of
host arch. I've attached a patch to this effect, and would welcome
feedback from testers.

> audio seems to work better now. there are a few glitches but at
> least it's not white noise. oh and export to mpeg2 and wav bot
> work.

Great. We're getting closer, it seems. Does the video part of mpeg2
export also work correctly now?

Paul, thanks a lot for looking into this issue!

Regards,

Daniel.

#! /bin/sh /usr/share/dpatch/dpatch-run
## 40_yuvdisplay_endian_fixes.dpatch by Daniel Kobras <kobras@debian.org>
##
## All lines beginning with `## DP:' are a description of the patch.
## DP: Always unroll YUV formats in same order, independent of host
## DP: endianness.

@DPATCH@
diff -urNad kino/src/frame.cc /tmp/dpep.thGYPG/kino/src/frame.cc
--- kino/src/frame.cc	2005-03-25 00:52:58.000000000 +0100
+++ /tmp/dpep.thGYPG/kino/src/frame.cc	2005-03-25 00:57:04.000000000 +0100
@@ -1052,12 +1052,10 @@
 
 			for ( int x = 0; x < width; x += 2 )
 			{
-				*reinterpret_cast<uint32_t*>( dest ) = Y[ 0 ] + ( Cb[ 0 ] << 8 ) + ( Y[ 1 ] << 16 ) + ( Cr[ 0 ] << 24 );
-
-				dest += 4;
-				Y += 2;
-				++Cb;
-				++Cr;
+				*dest++ = *Y++;
+				*dest++ = *Cb++;
+				*dest++ = *Y++;
+				*dest++ = *Cr++;
 			}
 		}
 	}
@@ -1071,13 +1069,14 @@
 
 			for ( int x = 0; x < width; x += 4 )
 			{
-				*reinterpret_cast<uint32_t*>( dest ) = Y[ 0 ] + ( Cb[ 0 ] << 8 ) + ( Y[ 1 ] << 16 ) + ( Cr[ 0 ] << 24 );
-				*reinterpret_cast<uint32_t*>( dest + 4 ) = Y[ 2 ] + ( Cb[ 0 ] << 8 ) + ( Y[ 3 ] << 16 ) + ( Cr[ 0 ] << 24 );
-
-				dest += 8;
-				Y += 4;
-				++Cb;
-				++Cr;
+				*dest++ = *Y++;
+				*dest++ = *Cb;
+				*dest++ = *Y++;
+				*dest++ = *Cr;
+				*dest++ = *Y++;
+				*dest++ = *Cb++;
+				*dest++ = *Y++;
+				*dest++ = *Cr++;
 			}
 		}
 	}