[MPlayer-dev-eng] Solaris patches

[Attila Kinali]

On Sun, 03 Sep 2006 01:50:07 +0100
Robin KAY <komadori at gekkou.co.uk> wrote:

> I'm the maintainer of blastwave.org's mplayer package. I have made some 
> patches in the course of my work. They are available both against 
> 1.0pre8 and the current SVN head.

Heh.. I wondered when patches from blastwave would finaly come :-)

I guess you have read DOCS/tech/patches.txt ?
You have a lot of cosmetics in your patches which should not
be there.

I wont comment on the pre8 patches, as pre8 will not
get any functional changes anymore

> http://www.blastwave.org/~komadori/patches/mplayer-head-20060902-fix-big-endian-colours.diff

diff -ru mplayer-head-20060902/libvo/vo_x11.c mplayer-head-20060902/libvo/vo_x11.c
--- mplayer-head-20060902-orig/libvo/vo_x11.c	Sat Sep  2 19:16:18 2006
+++ mplayer-head-20060902/libvo/vo_x11.c	Sat Sep  2 20:53:02 2006
@@ -472,27 +472,43 @@
         vo_dheight = vo_screenheight;
     }
 
+    rmask = myximage->red_mask;
+    gmask = myximage->green_mask;
+    bmask = myximage->blue_mask;
+#ifdef WORDS_BIGENDIAN
+    if (myximage->byte_order != MSBFirst)
+#else
+    if (myximage->byte_order != LSBFirst)
+#endif
+    {
+      rmask = bswap_32(rmask);
+      gmask = bswap_32(gmask);
+      bmask = bswap_32(bmask);
+    }

You should make this rather in the form of
if (..)
{ ..}
else
{...}
This would make it clearer what's going on.

+        case 16:
+            draw_alpha_fnc = draw_alpha_16;
+            out_format = IMGFMT_BGR16;
             break;
         case 15:
-        case 16:
-            if (depth == 15)
-            {
-                draw_alpha_fnc = draw_alpha_15;
-                out_format = IMGFMT_BGR15;
-            } else
-            {
-                draw_alpha_fnc = draw_alpha_16;
-                out_format = IMGFMT_BGR16;
-            }
+            draw_alpha_fnc = draw_alpha_15;
+            out_format = IMGFMT_BGR15;
             break;
         case 8:
             draw_alpha_fnc = draw_alpha_null;

This is an unrelated change and can be counted
as cosmetics. It is valid though. Please split
it into a seperate patch.

@@ -512,35 +535,6 @@
     dst_width = width;
     //printf( "X11 bpp: %d  color mask:  R:%lX  G:%lX  B:%lX\n",bpp,myximage->red_mask,myximage->green_mask,myximage->blue_mask );
 
-    // If we have blue in the lowest bit then obviously RGB
-    mode = ((myximage->blue_mask & 0x01) != 0) ? MODE_RGB : MODE_BGR;
-#ifdef WORDS_BIGENDIAN
-    if (myximage->byte_order != MSBFirst)
-#else
-    if (myximage->byte_order != LSBFirst)
-#endif
-    {
-        mode = ((myximage->blue_mask & 0x01) != 0) ? MODE_BGR : MODE_RGB;
-//   printf( "No support for non-native XImage byte order!\n" );
-//   return -1;
-    }
-#ifdef WORDS_BIGENDIAN
-    if (mode == MODE_BGR && bpp != 32)
-    {
-        mp_msg(MSGT_VO, MSGL_ERR,
-               "BGR%d not supported, please contact the developers\n",
-               bpp);
-        return -1;
-    }
-#else
-    if (mode == MODE_BGR)
-    {
-        mp_msg(MSGT_VO, MSGL_ERR,
-               "BGR not supported, please contact the developers\n");
-        return -1;
-    }
-#endif
-

Can you explain why this doesn't work for you?
It apreantly works for everone else on a big endian machine.
(including my tests on solaris/sparc)

> http://www.blastwave.org/~komadori/patches/mplayer-head-20060902-fix-solaris8-errors.diff

What error is fixed here?

> http://www.blastwave.org/~komadori/patches/mplayer-head-20060902-fix-unaligned-read.diff
> 
> Fixes a crash on SPARC in the ASF demuxer.

Using memcpy to fix alignment here looks bogus to me.
But i'm not sure what the correct way to do it would be.
I've never dealt with such issues.

				Attila Kinali
-- 
egp ist vergleichbar mit einem ikea bausatz fuer flugzeugtraeger
			-- reeler in +kaosu