[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [pygame] BUG: pygame.Color bugs - 64bit. Re: beginning GSOC preparations: was Re: [pygame] Thank You



Lenard,

Thanks a lot! :) I incorporated your fix into the baseline code, and there just needed to be some slight modifications for the other conversion functions, long, int, float, in addition to float and oct. I had initially considered LONG_MAX, but I wasn't sure what it would do, and didn't know if it would break 32 bit code. Now I know. Thanks. :) Here's an updated patch that covers all the conversion functions.

-Tyler

On Fri, May 1, 2009 at 9:34 PM, Lenard Lindstrom <len-l@xxxxxxxxx> wrote:
Hi,

Yes, I was using the second patch which only modifies color.c. However the 'L's are necessary on 32 bit machines since 0xCC00CC00 can only be represented as a Python long. So try this patch. It keeps the "L"s but replaces INT_MAX with LONG_MAX, which is the proper test for a Python int (*).

Lenard

(*) ints are longs and longs are, well, something else.

Tyler Laing wrote:
Are you using the second patch I provided? I had to make some modifications initially to the color_test.py, which I had a feeling would break on 32 bit machines. The second patch avoids modifying color_test.py, and instead does not cast the results from hex and oct as longs, which was adding a 'L' to the end of the result, but rather as ints every time.

-Tyler

On Fri, May 1, 2009 at 8:41 PM, Lenard Lindstrom <len-l@xxxxxxxxx <mailto:len-l@xxxxxxxxx>> wrote:

   Hi Tyler,

   This patch breaks things for 32bit machines. Give me a few minutes
   and I will see if I can cook something up.

   Lenard


   Tyler Laing wrote:

       Rene,

       Oops, about the windows line ends. I'll make sure that doesn't
       happen again. Sorry about that. I do have SVN write access
       now, I just wanted to start with the patch first.

       I honestly don't know much about pygame.color, so I can't help
       there.

       However, if we never want to return a long, then, we can change:

       /**
        * oct(color)
        */
       static PyObject*
       _color_oct (PyColor *color)
       {
          char buf[100];
          unsigned long tmp = ((long)color->r << 24) +
       ((long)color->g << 16) + ((long)color->b << 8) +
              color->a;
          if (tmp < INT_MAX)
              PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
          else
              PyOS_snprintf (buf, sizeof (buf), "0%loL", tmp);
          return PyString_FromString (buf);
       }

       to
       static PyObject*
       _color_oct (PyColor *color)
       {
          char buf[100];
          unsigned long tmp = ((long)color->r << 24) +
       ((long)color->g << 16) + ((long)color->b << 8) +
              color->a;
          if (tmp < INT_MAX)
              PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
          else
              PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
          return PyString_FromString (buf);
       }

       as an example, where we remove the 'L' from the else branch.

       I've got a fixed patch.diff now, where we don't return longs,
       and its a lot smaller, because color_test.py does not need the
       fixes now.

       I only have one error in the tests, and that results from not
       having the right permissions, which is solved by a simple sudo.

       -Tyler

       On Fri, May 1, 2009 at 6:59 PM, René Dudfield
       <renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>
       <mailto:renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>>> wrote:

          Cool, thanks.

          I think we can look applying this patch after Lenard has
       merged his
          py3k stuff back in... so we don't get any many conflicts.  As I
          noticed he's already touched the color.c.

          We shouldn't ever be returning a long I don't think.  So
       perhaps we
          can do a conversion from python long to python int at the
       end of the
          functions.

          For endian checking, you can use this define...

          #if SDL_BYTEORDER == SDL_LIL_ENDIAN
          // lil endian code here...
          #else
          // big endian code here...
          #endif

          However, I'm not positive the color code *should* be endian
       safe...
          without using the map/unmap methods on Surface for example.
        However I
          think maybe it should be.  We will have to study how it is
       being used
          at the moment... and how the old behaviour worked(as some
       code might
          rely on it being endian unsafe).


          ps.  with patches it's good to check them to make sure
       they're only
          for the change you made... not for other issues at the same
       time.  eg,
          there were lots of windows end of line characters in the
       color test
          before, and your patch removes them.

          pps. just a note, that a request has been put in to the
       lovely server
          administrators at seul.org <http://seul.org>
       <http://seul.org> for adding svn

          accounts for the all the
          GSOC peoples.  So you should get your svn account within a
       week or so.



          cu,





          On Sat, May 2, 2009 at 11:42 AM, Tyler Laing
       <trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>
          <mailto:trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>>>

       wrote:
          > Hmm, just noticed another issue. I had to make the change
       from
          INT_MAX to
          > LONG_MAX because the outputs for the specific number had
       an 'L'
          on the end
          > of them, where the number exceeds INT_MAX, and so a 'L' is
          appended to the
          > end of the outputs from oct and hex, which causes a lot of
          fails, as when
          > Python converts the numbers to hex/int, there is no 'L'
       on the
          end of that
          > value.
          >
          > Basically this line:
          >
          > self.assertEquals (hex (c), str(hex (0xCC00CC11))
          >
          > fails with this message:
          >
          >
                ======================================================================
          > FAIL: ColorTypeTest.test_webstyle
          >
                ----------------------------------------------------------------------
          > Traceback (most recent call last):
          >   File
                "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
       line
          > 465, in test_webstyle
          >     self.assertEquals (hex (c), hex (0xCC00CC11))
          > AssertionError: '0xcc00cc11L' != '0xcc00cc11'
          >
          > My fix, which I do not like, at all, is:
          >
          > self.assertEquals (str(hex (c))[:-1], str(hex (0xCC00CCFF)))
          >
          > I don't know if this would behave differently on a 32 bit
       platform.
          >
          > Here is the patch.diff.
          >
          > -Tyler
          >
          > On Fri, May 1, 2009 at 6:23 PM, René Dudfield
       <renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>
          <mailto:renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>>> wrote:
          >>
          >> Hi,
          >>
          >> cool, nice one.
          >>
          >> make your change to the file/files, make sure it
       compiles and
          passes
          >> the tests...
          >>
          >> Then run: svn diff > patch.diff
          >>
          >> Then send the patch to the mailing list... or upload it
          somewhere, and
          >> send a link to it if it's really big...  or just paste
       it into the
          >> email if it's tiny.
          >>
          >> cu,
          >>
          >>
          >>
          >>
          >> On Sat, May 2, 2009 at 11:21 AM, Tyler Laing
          <trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>
       <mailto:trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>>> wrote:
          >> > Rene,
          >> >
          >> > Okay, so I've got a fix. You have to prefix color->r with
          (long), and
          >> > then
          >> > for hex and oct functions, you need to change INT_MAX to
          LONG_MAX for a
          >> > 64
          >> > bit platform.
          >> >
          >> > How do I make a patch for submission?
          >> >
          >> > -Tyler
          >> >
          >> > On Fri, May 1, 2009 at 5:33 PM, Tyler Laing
          <trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>
       <mailto:trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>>> wrote:
          >> >>
          >> >> Rene,
          >> >>
          >> >> You are right. I isolated the specific issue, and
       here's a
          sample .c
          >> >> file
          >> >> that shows the error on the 64 bit platform. When I get
          something that
          >> >> works
          >> >> on the the test file, I'll try it on the actual
       pygame code
          and see how
          >> >> the
          >> >> test performs.
          >> >>
          >> >> -Tyler
          >> >>
          >> >> On Fri, May 1, 2009 at 4:30 PM, Tyler Laing
          <trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>
       <mailto:trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>>>


          >> >> wrote:
          >> >>>
          >> >>> I'll try that then. For reference, I am using an AMD
       Athlon
          64 X2
          >> >>> 5200+
          >> >>> processor. What would be the proper way to make it
       endian safe?
          >> >>>
          >> >>> Change the unsigned long tmp to unsigned int tmp?
          >> >>>
          >> >>> -Tyler
          >> >>>
          >> >>> On Fri, May 1, 2009 at 4:24 PM, René Dudfield
          <renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>
       <mailto:renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>>>


          >> >>> wrote:
          >> >>>>
          >> >>>> hi,
          >> >>>>
          >> >>>> there's some parts like this...
          >> >>>>
          >> >>>> static PyObject*
          >> >>>> _color_float (PyColor *color)
          >> >>>> {
          >> >>>>    unsigned long tmp = (color->r << 24) + (color->g
       << 16) +
          >> >>>> (color->b
          >> >>>> << 8) +
          >> >>>>        color->a;
          >> >>>>    return PyFloat_FromDouble ((double) tmp);
          >> >>>> }
          >> >>>>
          >> >>>> this code isn't endian or 64bit safe... since it is
       using bit
          >> >>>> shifting
          >> >>>> for packing.  On different platforms, this produces
       different
          >> >>>> outputs.
          >> >>>>
          >> >>>> I think it has to convert into the same 32bit unsigned
          int, and then
          >> >>>> return that.
          >> >>>>
          >> >>>>
          >> >>>>
          >> >>>>
          >> >>>> On Sat, May 2, 2009 at 8:54 AM, Tyler Laing
          <trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>
       <mailto:trinioler@xxxxxxxxx <mailto:trinioler@xxxxxxxxx>>>


          >> >>>> wrote:
          >> >>>> > Taking a look at color.c, I believe the bug may
       actually
          rest in
          >> >>>> > the
          >> >>>> >
                Py<type>_FromUnsignedLong/Py<type>_FromDouble/Py<type>_FromString
          >> >>>> > functions
          >> >>>> > provided by the Python libs. There is no logical or
          numerical
          >> >>>> > reason
          >> >>>> > why,
          >> >>>> > from the numbers we have, we would get those
       values with
          those
          >> >>>> > operations.
          >> >>>> > The tests beforehand affirm that the r, g, b, and a
          variables all
          >> >>>> > the
          >> >>>> > proper
          >> >>>> > values, it just happens to be the one step in the
       code. I'll
          >> >>>> > examine
          >> >>>> > further.
          >> >>>> >
          >> >>>> > -Tyler
          >> >>>> >
          >> >>>> > On Fri, May 1, 2009 at 3:28 PM, René Dudfield
          <renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>
       <mailto:renesd@xxxxxxxxx <mailto:renesd@xxxxxxxxx>>>


          >> >>>> > wrote:
          >> >>>> >>
          >> >>>> >> hi,
          >> >>>> >>
          >> >>>> >> Below are the failing unittests for Color on
       64bit ubuntu.
          >> >>>> >>
          >> >>>> >>
          >> >>>> >>
          >> >>>> >>
          >> >>>> >>
          >> >>>> >>
          >> >>>> >>
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_float
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 412, in test_float
          >> >>>> >> >     self.assertEquals (float (c), float
       (0xCC00CC00))
          >> >>>> >> > AssertionError: 1.844674407283719e+19 !=
       3422604288.0
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_hex
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 442, in test_hex
          >> >>>> >> >     self.assertEquals (hex (c), hex (0xCC00CC00))
          >> >>>> >> > AssertionError: '0xffffffffcc00cc00L' !=
       '0xcc00cc00'
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_int
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 494, in test_int
          >> >>>> >> >     self.assertEquals (int (c), int (0xCC00CC00))
          >> >>>> >> > AssertionError: 18446744072837188608L !=
       3422604288
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_long
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 511, in test_long
          >> >>>> >> >     self.assertEquals (long (c), long
       (0xCC00CC00))
          >> >>>> >> > AssertionError: 18446744072837188608L !=
       3422604288L
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_oct
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 427, in test_oct
          >> >>>> >> >     self.assertEquals (oct (c), oct (0xCC00CC00))
          >> >>>> >> > AssertionError: '01777777777771400146000L' !=
          '031400146000'
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ======================================================================
          >> >>>> >> > FAIL: ColorTypeTest.test_webstyle
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >> >
                ----------------------------------------------------------------------
          >> >>>> >> > Traceback (most recent call last):
          >> >>>> >> >   File
          >> >>>> >> >
          "/usr/lib/python2.5/site-packages/pygame/tests/color_test.py",
          >> >>>> >> > line
          >> >>>> >> > 458, in test_webstyle
          >> >>>> >> >     self.assertEquals (hex (c), hex (0xCC00CC11))
          >> >>>> >> > AssertionError: '0xffffffffcc00cc11L' !=
       '0xcc00cc11'
          >> >>>> >> >
          >> >>>> >> >
          >> >>>> >






--
Visit my blog at http://oddco.ca/zeroth/zblog
Index: src/color.c
===================================================================
--- src/color.c	(revision 2051)
+++ src/color.c	(working copy)
@@ -1407,7 +1407,7 @@
 static PyObject*
 _color_int (PyColor *color)
 {
-    unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+    unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
         color->a;
 #if !PY3
     if (tmp < INT_MAX)
@@ -1422,7 +1422,7 @@
 static PyObject*
 _color_long (PyColor *color)
 {
-    unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+    unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
         color->a;
     return PyLong_FromUnsignedLong (tmp);
 }
@@ -1433,7 +1433,7 @@
 static PyObject*
 _color_float (PyColor *color)
 {
-    unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+    unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
         color->a;
     return PyFloat_FromDouble ((double) tmp);
 }
@@ -1446,9 +1446,9 @@
 _color_oct (PyColor *color)
 {
     char buf[100];
-    unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+    unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
         color->a;
-    if (tmp < INT_MAX)
+    if (tmp < LONG_MAX)
         PyOS_snprintf (buf, sizeof (buf), "0%lo", tmp);
     else
         PyOS_snprintf (buf, sizeof (buf), "0%loL", tmp);
@@ -1462,9 +1462,9 @@
 _color_hex (PyColor *color)
 {
     char buf[100];
-    unsigned long tmp = (color->r << 24) + (color->g << 16) + (color->b << 8) +
+    unsigned long tmp = ((unsigned long)color->r << 24) + (color->g << 16) + (color->b << 8) +
         color->a;
-    if (tmp < INT_MAX)
+    if (tmp < LONG_MAX)
         PyOS_snprintf (buf, sizeof (buf), "0x%lx", tmp);
     else
     {
@@ -1475,7 +1475,7 @@
         PyOS_snprintf (buf, sizeof (buf), "0x%lXL", tmp);
 #endif
     }
-    return Text_FromUTF8 (buf);
+    return PyString_FromString (buf);
 }
 #endif