ImageData.get_data() throws an exception on valid list input

[GoogleCodeExporter]

The following code causes an exception:

import pyglet
data = [1,2,3,4] * 10 * 10
a = pyglet.image.ImageData(10, 10, 'RGBA', data)
a.get_data('RGBA', 4)

The exception trace is:
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/pymodules/python2.7/pyglet/image/__init__.py", line 701, in get_data
    return self._convert(format, pitch)
  File "/usr/lib/pymodules/python2.7/pyglet/image/__init__.py", line 986, in _convert
    self._ensure_string_data()
  File "/usr/lib/pymodules/python2.7/pyglet/image/__init__.py", line 1053, in _ensure_string_data
    memmove(buf, self._current_data, len(self._current_data))
ctypes.ArgumentError: argument 2: <type 'exceptions.TypeError'>: wrong type

This behavior occurs using both the last stable version and tip, using Python 
2.7.7 on 64-bit Linux (other platforms not tested).  

Contrast this with:

import pyglet
data = 'abcd' * 10 * 10
a = pyglet.image.ImageData(10, 10, 'RGBA', data)
a.get_data('RGBA', 4)

Which does not result in an error.

Did not try to run it under Python 3 since the current tip of pyglet doesn't 
appear to install successfully in Python 3.

Output of `python -m pyglet.info` is attached.

Original issue reported on code.google.com by icefo...@gmail.com on 23 Jun 2014 at 10:51

Attachments:

• pyglet.info.out

[GoogleCodeExporter]

The docstrings are wrong here.

Looking at the code, the data must pass trough 
pyglet.image.ImageData._ensure_string_data, which is

def _ensure_string_data(self):
    if type(self._current_data) is not bytes_type:
        buf = create_string_buffer(len(self._current_data))
        memmove(buf, self._current_data, len(self._current_data))
        self._current_data = buf.raw

So 'data' must be suitable for the call
    ctypes.memmove(buf, data, len(data))

ctypes.memmove assumes a contiguous C byte buffer, so python lists are not 
suitable

Minimal patch
=============

A minimal patch to close the issue would only correct the docstrings, something 
like the attached as issue_#752_minimal_patch.diff

More exploration
================

A bit of experimentation shows that two common python wrappers for C buffers, 
'bytearray' and "array.array('B',...)" are not accepted, while type 
T=ctypes.c_char * size is accepted.

Additionally, with the last type the code performs one unnecessary copy: first 
it explicitly copies with the memmove, later an implicit copy is made by python 
when referencing buf.raw ( see ctypes docs, or make a buffer an compare id(a) 
id(b) after doing a = buf.raw; b = buf.raw )

This suggest to change to:

def _ensure_string_data(self):
    if type(self._current_data) is not bytes_type:
        T = c_char * len(self._current_data)
        # binds buf to the buffer into self._current_data (no copy)
        buf = T.from_buffer(self._current_data)
        # buf raw generates a new str, filled with the contents of buf
        # (implicit copy)
        self._current_data = buf.raw

This allows to pass bytearray and array.array('B', ...) types, and type 
T=ctypes.c_char * size continues working. Also, no innecesary copy is performed.

In fact, the body's if will accept anything supporting the writable buffer 
protocol (ref: ctypes docs)

I don't know if they are relevant types accepted by the original code but not 
by the later but:

   - trying a number of pyglet samples and tests, (including video.py), they all work; also the cocos's autotest suite (200+ tests) runs normal with this change

   - If need arises, a proper conditional branch for other types can be added later, and then the tests should show which extra types are needed

This lends to a

Medium patch
============    

Includes the suggested modification plus tests and docstrings, attached as 
issue_#752_medium_patch.diff

Tested in py2.6 and py 3.3

Original comment by ccanepacc@gmail.com on 15 Jul 2014 at 5:12

Attachments:

• issue_#752_minimal_patch.diff
• issue_#752_medium_patch.diff

[GoogleCodeExporter]

Original comment by useboxnet on 16 Jul 2014 at 4:50

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by useboxnet on 16 Jul 2014 at 4:50

• Added labels: OpSys-All, Type-Patch