From e4b0dbe42edc160672dc7c3ca7e44ddb792811e3 Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:00:51 +0200 Subject: [PATCH 01/10] Use namedtuples for load_animation --- buildconfig/stubs/pygame/image.pyi | 14 +++++++----- src_c/imageext.c | 36 ++++++++++++++++++++++++++---- test/image_test.py | 3 +++ 3 files changed, 44 insertions(+), 9 deletions(-) diff --git a/buildconfig/stubs/pygame/image.pyi b/buildconfig/stubs/pygame/image.pyi index b9e1d487d0..a5cd28cec0 100644 --- a/buildconfig/stubs/pygame/image.pyi +++ b/buildconfig/stubs/pygame/image.pyi @@ -62,9 +62,8 @@ following formats. .. versionaddedold:: 1.8 Saving PNG and JPEG files. """ -from typing import Literal, Optional, Union +from typing import Literal, NamedTuple, Optional -from pygame.bufferproxy import BufferProxy from pygame.surface import Surface from pygame.typing import FileLike, IntPoint, Point from typing_extensions import ( @@ -78,6 +77,10 @@ _to_bytes_format = Literal[ ] _from_bytes_format = Literal["P", "RGB", "RGBX", "RGBA", "ARGB", "BGRA", "ABGR"] +class _AnimationFrame(NamedTuple): + surface: Surface + delay: int + def load(file: FileLike, namehint: str = "") -> Surface: """Load new image from a file (or file-like object). @@ -136,7 +139,7 @@ def load_sized_svg(file: FileLike, size: Point) -> Surface: .. versionadded:: 2.4.0 """ -def load_animation(file: FileLike, namehint: str = "") -> list[tuple[Surface, int]]: +def load_animation(file: FileLike, namehint: str = "") -> list[_AnimationFrame]: """Load an animation (GIF/WEBP) from a file (or file-like object). Load an animation (GIF/WEBP) from a file source. You can pass either a @@ -145,8 +148,9 @@ def load_animation(file: FileLike, namehint: str = "") -> list[tuple[Surface, in namehint argument so that the file extension can be used to infer the file format. - This returns a list of tuples (corresponding to every frame of the animation), - where each tuple is a (surface, delay) pair for that frame. + This returns a list of named tuples (corresponding to every frame of the animation), + where each tuple is a (surface, delay) pair for that frame. Being named tuples, + the items can be accessed as frame.surface and frame.delay. This function requires SDL_image 2.6.0 or above. If pygame was compiled with an older version, ``pygame.error`` will be raised when this function is diff --git a/src_c/imageext.c b/src_c/imageext.c index 5e39e62d02..7f5b62fb61 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -69,6 +69,7 @@ static SDL_mutex *_pg_img_mutex = 0; #endif */ +static PyTypeObject *animation_frame_type = NULL; static char * iext_find_extension(char *fullname) @@ -216,6 +217,8 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) SDL_RWops *rw = NULL; static char *kwds[] = {"file", "namehint", NULL}; + assert(animation_frame_type); + if (!PyArg_ParseTupleAndKeywords(arg, kwargs, "O|s", kwds, &obj, &name)) { return NULL; } @@ -263,12 +266,22 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) /* The python surface object now "owns" the sdl surface, so set it * to null in the animation to prevent double free */ surfs->frames[i] = NULL; - - PyObject *listentry = Py_BuildValue("(Oi)", frame, surfs->delays[i]); - Py_DECREF(frame); + PyObject *listentry = PyStructSequence_New(animation_frame_type); if (!listentry) { + Py_DECREF(frame); + goto error; + } + + PyStructSequence_SetItem(listentry, 0, frame); + PyObject *delay_obj = PyLong_FromLong(surfs->delays[i]); + if (!delay_obj) { + Py_DECREF(listentry); + Py_DECREF(frame); goto error; } + PyStructSequence_SetItem(listentry, 1, delay_obj); + Py_DECREF(delay_obj); + PyList_SET_ITEM(ret, i, listentry); } IMG_FreeAnimation(surfs); @@ -461,7 +474,6 @@ MODINIT_DEFINE(imageext) return NULL; } import_pygame_rwobject(); - if (PyErr_Occurred()) { return NULL; } @@ -476,6 +488,22 @@ MODINIT_DEFINE(imageext) #endif */ + static PyStructSequence_Field _namedtuple_fields[] = { + {"surface", NULL}, {"delay", NULL}, NULL}; + + static struct PyStructSequence_Desc _namedtuple_descr = { + .name = "AnimationFrame", + .doc = NULL, + .fields = _namedtuple_fields, + .n_in_sequence = 2, + }; + + animation_frame_type = PyStructSequence_NewType(&_namedtuple_descr); + if (animation_frame_type == NULL) { + return NULL; // exception set + } + Py_INCREF(animation_frame_type); + /* create the module */ return PyModule_Create(&_module); } diff --git a/test/image_test.py b/test/image_test.py index 05520c85d2..79a010b776 100644 --- a/test/image_test.py +++ b/test/image_test.py @@ -1388,6 +1388,9 @@ def test_load_animation(self): for val in s: self.assertIsInstance(val, tuple) frame, delay = val + frameattr, delayattr = val.surface, val.delay + self.assertEqual(frame, frameattr) + self.assertEqual(delay, delayattr) self.assertIsInstance(frame, pygame.Surface) self.assertEqual(frame.size, SAMPLE_SIZE) self.assertIsInstance(delay, int) From c1c3ff0499c81cf589309ae5c49138ceb9cf843e Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:08:01 +0200 Subject: [PATCH 02/10] Update doc header --- src_c/doc/image_doc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src_c/doc/image_doc.h b/src_c/doc/image_doc.h index bbb1f16c45..ea4a012336 100644 --- a/src_c/doc/image_doc.h +++ b/src_c/doc/image_doc.h @@ -2,7 +2,7 @@ #define DOC_IMAGE "Pygame module for image transfer." #define DOC_IMAGE_LOAD "load(file, namehint='') -> Surface\nLoad new image from a file (or file-like object)." #define DOC_IMAGE_LOADSIZEDSVG "load_sized_svg(file, size) -> Surface\nLoad an SVG image from a file (or file-like object) with the given size." -#define DOC_IMAGE_LOADANIMATION "load_animation(file, namehint='') -> list[tuple[Surface, int]]\nLoad an animation (GIF/WEBP) from a file (or file-like object)." +#define DOC_IMAGE_LOADANIMATION "load_animation(file, namehint='') -> list[_AnimationFrame]\nLoad an animation (GIF/WEBP) from a file (or file-like object)." #define DOC_IMAGE_SAVE "save(surface, file, namehint='') -> None\nSave an image to file (or file-like object)." #define DOC_IMAGE_GETSDLIMAGEVERSION "get_sdl_image_version(linked=True) -> Optional[tuple[int, int, int]]\nGet version number of the SDL_Image library being used." #define DOC_IMAGE_GETEXTENDED "get_extended() -> bool\nTest if extended image formats can be loaded." From 72a5fdc27920b0191379001ec77ada0bac157974 Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:12:05 +0200 Subject: [PATCH 03/10] Correctly end the array --- src_c/imageext.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src_c/imageext.c b/src_c/imageext.c index 7f5b62fb61..8015c80324 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -489,7 +489,7 @@ MODINIT_DEFINE(imageext) */ static PyStructSequence_Field _namedtuple_fields[] = { - {"surface", NULL}, {"delay", NULL}, NULL}; + {"surface", NULL}, {"delay", NULL}, {NULL, NULL}}; static struct PyStructSequence_Desc _namedtuple_descr = { .name = "AnimationFrame", From f0867ca9915b435abea2264c21c6a97d2ff53e14 Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:33:23 +0200 Subject: [PATCH 04/10] Improve from suggestions --- src_c/imageext.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src_c/imageext.c b/src_c/imageext.c index 8015c80324..58b62973f0 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -217,8 +217,6 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) SDL_RWops *rw = NULL; static char *kwds[] = {"file", "namehint", NULL}; - assert(animation_frame_type); - if (!PyArg_ParseTupleAndKeywords(arg, kwargs, "O|s", kwds, &obj, &name)) { return NULL; } @@ -257,6 +255,10 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) } for (int i = 0; i < surfs->count; i++) { + PyObject *delay_obj = PyLong_FromLong(surfs->delays[i]); + if (!delay_obj) { + goto error; + } PyObject *frame = (PyObject *)pgSurface_New(surfs->frames[i]); if (!frame) { /* IMG_FreeAnimation takes care of freeing of member SDL surfaces @@ -273,15 +275,7 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) } PyStructSequence_SetItem(listentry, 0, frame); - PyObject *delay_obj = PyLong_FromLong(surfs->delays[i]); - if (!delay_obj) { - Py_DECREF(listentry); - Py_DECREF(frame); - goto error; - } PyStructSequence_SetItem(listentry, 1, delay_obj); - Py_DECREF(delay_obj); - PyList_SET_ITEM(ret, i, listentry); } IMG_FreeAnimation(surfs); @@ -492,7 +486,7 @@ MODINIT_DEFINE(imageext) {"surface", NULL}, {"delay", NULL}, {NULL, NULL}}; static struct PyStructSequence_Desc _namedtuple_descr = { - .name = "AnimationFrame", + .name = "_AnimationFrame", .doc = NULL, .fields = _namedtuple_fields, .n_in_sequence = 2, From dcd8d7e79a458242d44acba82f08d4d0f1fc1603 Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:35:11 +0200 Subject: [PATCH 05/10] Remove incref --- src_c/imageext.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src_c/imageext.c b/src_c/imageext.c index 58b62973f0..f98c6f2f65 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -496,7 +496,6 @@ MODINIT_DEFINE(imageext) if (animation_frame_type == NULL) { return NULL; // exception set } - Py_INCREF(animation_frame_type); /* create the module */ return PyModule_Create(&_module); From 1f2a005061d1efa7dd52acf68ba0ae1cdce5aa05 Mon Sep 17 00:00:00 2001 From: damusss Date: Sat, 24 May 2025 14:37:21 +0200 Subject: [PATCH 06/10] Decref delay properly --- src_c/imageext.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src_c/imageext.c b/src_c/imageext.c index f98c6f2f65..6c7352478b 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -263,6 +263,7 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) if (!frame) { /* IMG_FreeAnimation takes care of freeing of member SDL surfaces */ + Py_DECREF(delay_obj); goto error; } /* The python surface object now "owns" the sdl surface, so set it @@ -271,6 +272,7 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) PyObject *listentry = PyStructSequence_New(animation_frame_type); if (!listentry) { Py_DECREF(frame); + Py_DECREF(delay_obj); goto error; } From 3619673f11df815580709a91be613a4be13eca6c Mon Sep 17 00:00:00 2001 From: damusss Date: Sun, 25 May 2025 10:32:19 +0200 Subject: [PATCH 07/10] Rename delay_ms (I need to push to change branch, I could make another clone but nah) --- buildconfig/stubs/pygame/image.pyi | 6 +++--- src_c/imageext.c | 2 +- test/image_test.py | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/buildconfig/stubs/pygame/image.pyi b/buildconfig/stubs/pygame/image.pyi index a5cd28cec0..325fe2d0ca 100644 --- a/buildconfig/stubs/pygame/image.pyi +++ b/buildconfig/stubs/pygame/image.pyi @@ -79,7 +79,7 @@ _from_bytes_format = Literal["P", "RGB", "RGBX", "RGBA", "ARGB", "BGRA", "ABGR"] class _AnimationFrame(NamedTuple): surface: Surface - delay: int + delay_ms: int def load(file: FileLike, namehint: str = "") -> Surface: """Load new image from a file (or file-like object). @@ -149,8 +149,8 @@ def load_animation(file: FileLike, namehint: str = "") -> list[_AnimationFrame]: format. This returns a list of named tuples (corresponding to every frame of the animation), - where each tuple is a (surface, delay) pair for that frame. Being named tuples, - the items can be accessed as frame.surface and frame.delay. + where each tuple is a (surface, delay in milliseconds) pair for that frame. Being + named tuples, the items can be accessed as frame.surface and frame.delay_ms. This function requires SDL_image 2.6.0 or above. If pygame was compiled with an older version, ``pygame.error`` will be raised when this function is diff --git a/src_c/imageext.c b/src_c/imageext.c index 6c7352478b..aacb1b447d 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -485,7 +485,7 @@ MODINIT_DEFINE(imageext) */ static PyStructSequence_Field _namedtuple_fields[] = { - {"surface", NULL}, {"delay", NULL}, {NULL, NULL}}; + {"surface", NULL}, {"delay_ms", NULL}, {NULL, NULL}}; static struct PyStructSequence_Desc _namedtuple_descr = { .name = "_AnimationFrame", diff --git a/test/image_test.py b/test/image_test.py index 79a010b776..0b37b900a5 100644 --- a/test/image_test.py +++ b/test/image_test.py @@ -1388,7 +1388,7 @@ def test_load_animation(self): for val in s: self.assertIsInstance(val, tuple) frame, delay = val - frameattr, delayattr = val.surface, val.delay + frameattr, delayattr = val.surface, val.delay_ms self.assertEqual(frame, frameattr) self.assertEqual(delay, delayattr) self.assertIsInstance(frame, pygame.Surface) From 78173f36cdd664ea4b47ace130f03924b2b6066d Mon Sep 17 00:00:00 2001 From: damusss Date: Sun, 25 May 2025 12:55:52 +0200 Subject: [PATCH 08/10] Use float instead of int --- src_c/imageext.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src_c/imageext.c b/src_c/imageext.c index aacb1b447d..ed7a6ffb36 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -255,7 +255,7 @@ imageext_load_animation(PyObject *self, PyObject *arg, PyObject *kwargs) } for (int i = 0; i < surfs->count; i++) { - PyObject *delay_obj = PyLong_FromLong(surfs->delays[i]); + PyObject *delay_obj = PyFloat_FromDouble((double)surfs->delays[i]); if (!delay_obj) { goto error; } From ef7890f0efe711bee6faa266991cfcc89db33ff8 Mon Sep 17 00:00:00 2001 From: damusss Date: Sun, 25 May 2025 12:57:54 +0200 Subject: [PATCH 09/10] _actually_ use float --- buildconfig/stubs/pygame/image.pyi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/buildconfig/stubs/pygame/image.pyi b/buildconfig/stubs/pygame/image.pyi index 325fe2d0ca..eb8c36bb17 100644 --- a/buildconfig/stubs/pygame/image.pyi +++ b/buildconfig/stubs/pygame/image.pyi @@ -79,7 +79,7 @@ _from_bytes_format = Literal["P", "RGB", "RGBX", "RGBA", "ARGB", "BGRA", "ABGR"] class _AnimationFrame(NamedTuple): surface: Surface - delay_ms: int + delay_ms: float def load(file: FileLike, namehint: str = "") -> Surface: """Load new image from a file (or file-like object). @@ -140,7 +140,7 @@ def load_sized_svg(file: FileLike, size: Point) -> Surface: """ def load_animation(file: FileLike, namehint: str = "") -> list[_AnimationFrame]: - """Load an animation (GIF/WEBP) from a file (or file-like object). + """Load an animation (GIF/WEBP) from a file (or file-like object) as a list of frames. Load an animation (GIF/WEBP) from a file source. You can pass either a filename, a Python file-like object, or a pathlib.Path. If you pass a raw From 43542c608950574bc7d69abda5e2c9a2465247eb Mon Sep 17 00:00:00 2001 From: damusss Date: Sun, 25 May 2025 13:35:05 +0200 Subject: [PATCH 10/10] Move static definitions outside of MODINIT --- src_c/imageext.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src_c/imageext.c b/src_c/imageext.c index ed7a6ffb36..36d96eb53c 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -446,6 +446,16 @@ static PyMethodDef _imageext_methods[] = { /*DOC*/ static char _imageext_doc[] = /*DOC*/ "additional image loaders"; +static PyStructSequence_Field _namedtuple_fields[] = { + {"surface", NULL}, {"delay_ms", NULL}, {NULL, NULL}}; + +static struct PyStructSequence_Desc _namedtuple_descr = { + .name = "_AnimationFrame", + .doc = NULL, + .fields = _namedtuple_fields, + .n_in_sequence = 2, +}; + MODINIT_DEFINE(imageext) { static struct PyModuleDef _module = {PyModuleDef_HEAD_INIT, @@ -484,16 +494,6 @@ MODINIT_DEFINE(imageext) #endif */ - static PyStructSequence_Field _namedtuple_fields[] = { - {"surface", NULL}, {"delay_ms", NULL}, {NULL, NULL}}; - - static struct PyStructSequence_Desc _namedtuple_descr = { - .name = "_AnimationFrame", - .doc = NULL, - .fields = _namedtuple_fields, - .n_in_sequence = 2, - }; - animation_frame_type = PyStructSequence_NewType(&_namedtuple_descr); if (animation_frame_type == NULL) { return NULL; // exception set