From eaaff642792a86f7505162d90c99a943b04eed5c Mon Sep 17 00:00:00 2001 From: Starbuck5 Date: Wed, 25 Aug 2021 15:50:34 -0700 Subject: [PATCH 1/6] Simplify pygame resource loaders --- src_c/_pygame.h | 2 +- src_c/image.c | 25 ++------ src_c/imageext.c | 129 +++++++--------------------------------- src_c/include/_pygame.h | 4 ++ src_c/music.c | 50 ++++++---------- src_c/rwobject.c | 43 +++++++++++++- 6 files changed, 90 insertions(+), 163 deletions(-) diff --git a/src_c/_pygame.h b/src_c/_pygame.h index aad4f760d2..013c4a39ec 100644 --- a/src_c/_pygame.h +++ b/src_c/_pygame.h @@ -344,7 +344,7 @@ struct pgColorObject { #define PYGAMEAPI_DISPLAY_NUMSLOTS 2 #define PYGAMEAPI_SURFACE_NUMSLOTS 4 #define PYGAMEAPI_SURFLOCK_NUMSLOTS 8 -#define PYGAMEAPI_RWOBJECT_NUMSLOTS 6 +#define PYGAMEAPI_RWOBJECT_NUMSLOTS 7 #define PYGAMEAPI_PIXELARRAY_NUMSLOTS 2 #define PYGAMEAPI_COLOR_NUMSLOTS 5 #define PYGAMEAPI_MATH_NUMSLOTS 2 diff --git a/src_c/image.c b/src_c/image.c index 933783768e..665f5a2636 100644 --- a/src_c/image.c +++ b/src_c/image.c @@ -79,29 +79,14 @@ image_load_basic(PyObject *self, PyObject *obj) PyObject *final; PyObject *oencoded; SDL_Surface *surf; - SDL_RWops *rw; - oencoded = pg_EncodeString(obj, "UTF-8", NULL, pgExc_SDLError); - if (oencoded == NULL) { + SDL_RWops *rw = pgRWops_FromObject(obj); + if (rw == NULL) { return NULL; } - - if (oencoded != Py_None) { - Py_BEGIN_ALLOW_THREADS; - surf = SDL_LoadBMP(Bytes_AS_STRING(oencoded)); - Py_END_ALLOW_THREADS; - Py_DECREF(oencoded); - } - else { - Py_DECREF(oencoded); - rw = pgRWops_FromFileObject(obj); - if (rw == NULL) { - return NULL; - } - Py_BEGIN_ALLOW_THREADS; - surf = SDL_LoadBMP_RW(rw, 1); - Py_END_ALLOW_THREADS; - } + Py_BEGIN_ALLOW_THREADS; + surf = SDL_LoadBMP_RW(rw, 1); + Py_END_ALLOW_THREADS; if (surf == NULL) { return RAISE(pgExc_SDLError, SDL_GetError()); diff --git a/src_c/imageext.c b/src_c/imageext.c index b588e7bdaa..35a88aba40 100644 --- a/src_c/imageext.c +++ b/src_c/imageext.c @@ -108,127 +108,44 @@ image_load_ext(PyObject *self, PyObject *arg) { PyObject *obj; PyObject *final; - PyObject *oencoded; - PyObject *oname; - size_t namelen; const char *name = NULL; - const char *cext; char *ext = NULL; SDL_Surface *surf; - SDL_RWops *rw; + SDL_RWops *rw = NULL; + int lock_mutex = 0; if (!PyArg_ParseTuple(arg, "O|s", &obj, &name)) { return NULL; } - oencoded = pg_EncodeString(obj, "UTF-8", NULL, pgExc_SDLError); - if (oencoded == NULL) { + rw = pgRWops_FromObject(obj); + if (rw == NULL) /* stop on NULL, error already set */ return NULL; - } - if (oencoded != Py_None) { - name = Bytes_AS_STRING(oencoded); + ext = pgRWops_GetFileExtension(rw); + if (name) /* override extension with namehint if given */ + ext = find_extension(name); + #ifdef WITH_THREAD - namelen = Bytes_GET_SIZE(oencoded); - Py_BEGIN_ALLOW_THREADS; - if (namelen > 4 && !strcasecmp(name + namelen - 4, ".gif")) { - /* using multiple threads does not work for (at least) SDL_image <= 2.0.4 */ - SDL_LockMutex(_pg_img_mutex); - surf = IMG_Load(name); - SDL_UnlockMutex(_pg_img_mutex); - } - else { - surf = IMG_Load(name); - } - Py_END_ALLOW_THREADS; -#else /* ~WITH_THREAD */ - surf = IMG_Load(name); -#endif /* WITH_THREAD */ - Py_DECREF(oencoded); + if (ext) + lock_mutex = !strcasecmp(ext, "gif"); + Py_BEGIN_ALLOW_THREADS; + if (0) { + /* using multiple threads does not work for (at least) SDL_image <= 2.0.4 */ + SDL_LockMutex(_pg_img_mutex); + surf = IMG_LoadTyped_RW(rw, 1, ext); + SDL_UnlockMutex(_pg_img_mutex); } else { -#ifdef WITH_THREAD - int lock_mutex = 0; -#endif /* WITH_THREAD */ - Py_DECREF(oencoded); - oencoded = NULL; -#if PY2 - if (name == NULL && PyFile_Check(obj)) { - oencoded = PyFile_Name(obj); - if (oencoded == NULL) { - /* This should never happen */ - return NULL; - } - Py_INCREF(oencoded); - name = Bytes_AS_STRING(oencoded); - } -#endif - if (name == NULL) { - oname = PyObject_GetAttrString(obj, "name"); - if (oname != NULL) { - oencoded = pg_EncodeString(oname, "UTF-8", NULL, NULL); - Py_DECREF(oname); - if (oencoded == NULL) { - return NULL; - } - if (oencoded != Py_None) { - name = Bytes_AS_STRING(oencoded); - } - } - else { - PyErr_Clear(); - } - } - rw = pgRWops_FromFileObject(obj); - if (rw == NULL) { - Py_XDECREF(oencoded); - return NULL; - } - - cext = find_extension(name); - if (cext != NULL) { - ext = (char *)PyMem_Malloc(strlen(cext) + 1); - if (ext == NULL) { - Py_XDECREF(oencoded); - return PyErr_NoMemory(); - } - strcpy(ext, cext); -#ifdef WITH_THREAD - lock_mutex = !strcasecmp(ext, "gif"); -#endif /* WITH_THREAD */ - } - Py_XDECREF(oencoded); -#ifdef WITH_THREAD - Py_BEGIN_ALLOW_THREADS; - if (lock_mutex) { - /* using multiple threads does not work for (at least) SDL_image <= 2.0.4 */ - SDL_LockMutex(_pg_img_mutex); - surf = IMG_LoadTyped_RW(rw, 1, ext); - SDL_UnlockMutex(_pg_img_mutex); - } - else { - surf = IMG_LoadTyped_RW(rw, 1, ext); - } - Py_END_ALLOW_THREADS; -#else /* ~WITH_THREAD */ surf = IMG_LoadTyped_RW(rw, 1, ext); -#endif /* ~WITH_THREAD */ - PyMem_Free(ext); } + Py_END_ALLOW_THREADS; +#else /* ~WITH_THREAD */ + surf = IMG_LoadTyped_RW(rw, 1, ext); +#endif /* ~WITH_THREAD */ + + if (surf == NULL) + return RAISE(pgExc_SDLError, IMG_GetError()); - if (surf == NULL){ - if (!strncmp(IMG_GetError(), "Couldn't open", 12)){ - SDL_ClearError(); -#if PY3 - PyErr_SetString(PyExc_FileNotFoundError, "No such file or directory."); -#else - PyErr_SetString(PyExc_IOError, "No such file or directory."); -#endif - return NULL; - } - else{ - return RAISE(pgExc_SDLError, IMG_GetError()); - } - } final = (PyObject *)pgSurface_New(surf); if (final == NULL) { SDL_FreeSurface(surf); diff --git a/src_c/include/_pygame.h b/src_c/include/_pygame.h index ef7be91671..35fc0952c2 100644 --- a/src_c/include/_pygame.h +++ b/src_c/include/_pygame.h @@ -538,6 +538,10 @@ typedef struct pgEventObject pgEventObject; (*(int (*)(SDL_RWops *)) \ PYGAMEAPI_GET_SLOT(rwobject, 5)) +#define pgRWops_GetFileExtension \ + (*(char * (*)(SDL_RWops *)) \ + PYGAMEAPI_GET_SLOT(rwobject, 6)) + #define import_pygame_rwobject() IMPORT_PYGAME_MODULE(rwobject) #endif diff --git a/src_c/music.c b/src_c/music.c index 71598d07c9..99b463b252 100644 --- a/src_c/music.c +++ b/src_c/music.c @@ -307,6 +307,9 @@ _get_type_from_hint(char *namehint) namehint = dot + 1; } } + else { + return type; + } /* Copied almost directly from SDL_mixer. Originally meant to check file extensions * to get a hint of what music type it should be. @@ -366,10 +369,10 @@ static PyObject * music_load(PyObject *self, PyObject *args) { PyObject *obj; - PyObject *oencoded; Mix_Music *new_music = NULL; - const char *name; + char* ext = NULL; char *namehint = NULL; + SDL_RWops *rw = NULL; if (!PyArg_ParseTuple(args, "O|s", &obj, &namehint)) { return NULL; @@ -377,42 +380,23 @@ music_load(PyObject *self, PyObject *args) MIXER_INIT_CHECK(); - oencoded = pg_EncodeString(obj, "UTF-8", NULL, pgExc_SDLError); - if (oencoded == Py_None) { - SDL_RWops *rw; - - Py_DECREF(oencoded); - if (!PG_CHECK_THREADS()) - return NULL; - rw = pgRWops_FromFileObject(obj); - if (rw == NULL) { - return NULL; - } - Py_BEGIN_ALLOW_THREADS -#if IS_SDLv1 - new_music = Mix_LoadMUS_RW(rw); -#else /* IS_SDLv2 */ - if (namehint) - new_music = Mix_LoadMUSType_RW(rw, _get_type_from_hint(namehint), SDL_TRUE); - else - new_music = Mix_LoadMUS_RW(rw, SDL_TRUE); -#endif /* IS_SDLv2 */ - Py_END_ALLOW_THREADS - } - else if (oencoded != NULL) { - name = Bytes_AS_STRING(oencoded); - Py_BEGIN_ALLOW_THREADS new_music = Mix_LoadMUS(name); - Py_END_ALLOW_THREADS Py_DECREF(oencoded); - } - else { + rw = pgRWops_FromObject(obj); + if (rw == NULL) /* stop on NULL, error already set */ return NULL; - } + ext = pgRWops_GetFileExtension(rw); + if (namehint) /* override extension with namehint if given */ + ext = namehint; + + Py_BEGIN_ALLOW_THREADS + new_music = Mix_LoadMUSType_RW(rw, _get_type_from_hint(ext), SDL_TRUE); + Py_END_ALLOW_THREADS if (new_music == NULL) { return RAISE(pgExc_SDLError, SDL_GetError()); } - Py_BEGIN_ALLOW_THREADS if (current_music != NULL) + Py_BEGIN_ALLOW_THREADS + if (current_music != NULL) { Mix_FreeMusic(current_music); current_music = NULL; @@ -423,7 +407,7 @@ music_load(PyObject *self, PyObject *args) } Py_END_ALLOW_THREADS - current_music = new_music; + current_music = new_music; Py_RETURN_NONE; } diff --git a/src_c/rwobject.c b/src_c/rwobject.c index 6d7d179f41..49a2a6c81f 100644 --- a/src_c/rwobject.c +++ b/src_c/rwobject.c @@ -314,6 +314,17 @@ pgRWops_IsFileObject(SDL_RWops *rw) return rw->close == _pg_rw_close; } +char* +pgRWops_GetFileExtension(SDL_RWops* rw) +{ + if (pgRWops_IsFileObject(rw)) { + return NULL; + } + else { + return rw->hidden.unknown.data1; + } +} + #if IS_SDLv2 static Sint64 _pg_rw_size(SDL_RWops *context) @@ -509,6 +520,9 @@ pgRWops_FromFileObject(PyObject *obj) helper->file = obj; Py_INCREF(obj); + /* Adding a helper to the hidden data to support file-like object RWops + * RWops from actual files use this space to store the file extension + * for later use */ rw->hidden.unknown.data1 = (void *)helper; #if IS_SDLv2 rw->size = _pg_rw_size; @@ -567,6 +581,7 @@ pgRWops_ReleaseObject(SDL_RWops *context) #endif /* WITH_THREAD */ } else { + free(context->hidden.unknown.data1); ret = SDL_RWclose(context); if (ret < 0) PyErr_SetString(PyExc_IOError, SDL_GetError()); @@ -722,22 +737,43 @@ _rwops_from_pystr(PyObject *obj) if (obj != NULL) { SDL_RWops *rw = NULL; PyObject *oencoded; + char* encoded = NULL; + + //longest extension is 4 chars, plus NULL at end + char* extension = malloc(sizeof(char)*5); + if (extension == NULL) { + return (SDL_RWops*)PyErr_NoMemory(); + } + *extension = NULL; + char* ext = NULL; + oencoded = pg_EncodeString(obj, "UTF-8", NULL, NULL); if (oencoded == NULL) { return NULL; } if (oencoded != Py_None) { - rw = SDL_RWFromFile(Bytes_AS_STRING(oencoded), "rb"); + encoded = Bytes_AS_STRING(oencoded); + rw = SDL_RWFromFile(encoded, "rb"); + ext = SDL_strrchr(encoded, '.'); + if (ext) { + ext++; + strcpy(extension, ext); + } } Py_DECREF(oencoded); if (rw) { + /* adding the extension to the hidden data for RWops from files */ + /* this is necessary to support loading functions that rely on + * file extensions in a convenient way. File-like objects use this + * field for a helper object. */ + rw->hidden.unknown.data1 = (void *)extension; return rw; } else { #if PY3 if (PyUnicode_Check(obj)) { - SDL_ClearError(); + SDL_ClearError(); PyErr_SetString(PyExc_FileNotFoundError, - "No such file or directory."); + "No such file or directory."); #else if (PyUnicode_Check(obj) || PyString_Check(obj)) { SDL_ClearError(); @@ -850,6 +886,7 @@ MODINIT_DEFINE(rwobject) c_api[3] = pg_EncodeString; c_api[4] = pgRWops_FromFileObject; c_api[5] = pgRWops_ReleaseObject; + c_api[6] = pgRWops_GetFileExtension; apiobj = encapsulate_api(c_api, "rwobject"); if (apiobj == NULL) { DECREF_MOD(module); From a335491f211eef3f43abff815bc74b6c960d1e29 Mon Sep 17 00:00:00 2001 From: Starbuck5 Date: Wed, 25 Aug 2021 16:17:11 -0700 Subject: [PATCH 2/6] Update c api docs [skip ci] --- docs/reST/c_api/rwobject.rst | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/reST/c_api/rwobject.rst b/docs/reST/c_api/rwobject.rst index d8047dffdb..f1f5a9faf2 100644 --- a/docs/reST/c_api/rwobject.rst +++ b/docs/reST/c_api/rwobject.rst @@ -31,11 +31,17 @@ Header file: src_c/include/pygame.h If threads are available, the Python GIL is acquired before calling any of the *obj* methods. On error raise a Python exception and return ``NULL``. -.. c:function:: int pgRWops_CheckObject(SDL_RWops *rw) +.. c:function:: int pgRWops_IsFileObject(SDL_RWops *rw) Return true if *rw* is a Python file-like object wrapper returned by :c:func:`pgRWops_FromObject` or :c:func:`pgRWops_FromFileObject`. +.. c:function:: char* pgRWops_GetFileExtension(SDL_RWops *rw) + + Return a string that contains the file extension of the original file + loaded into the SDL_RWops object, or NULL if the SDL_RWops object comes + from a file object. + .. c:function:: int pgRWops_ReleaseObject(SDL_RWops *context) Free a SDL_RWops struct. If it is attached to a Python file-like object, decrement its From 65ca360226ca4202e5be47e8d18281ae54d5671a Mon Sep 17 00:00:00 2001 From: Starbuck5 Date: Wed, 25 Aug 2021 17:29:09 -0700 Subject: [PATCH 3/6] Fix rwobject changes for py2 (hopefully) --- src_c/rwobject.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src_c/rwobject.c b/src_c/rwobject.c index 49a2a6c81f..ee6f146aeb 100644 --- a/src_c/rwobject.c +++ b/src_c/rwobject.c @@ -738,14 +738,14 @@ _rwops_from_pystr(PyObject *obj) SDL_RWops *rw = NULL; PyObject *oencoded; char* encoded = NULL; + char* ext = NULL; + char* extension = NULL; //longest extension is 4 chars, plus NULL at end - char* extension = malloc(sizeof(char)*5); + extension = malloc(sizeof(char)*5); if (extension == NULL) { return (SDL_RWops*)PyErr_NoMemory(); } - *extension = NULL; - char* ext = NULL; oencoded = pg_EncodeString(obj, "UTF-8", NULL, NULL); if (oencoded == NULL) { @@ -754,7 +754,7 @@ _rwops_from_pystr(PyObject *obj) if (oencoded != Py_None) { encoded = Bytes_AS_STRING(oencoded); rw = SDL_RWFromFile(encoded, "rb"); - ext = SDL_strrchr(encoded, '.'); + ext = strrchr(encoded, '.'); if (ext) { ext++; strcpy(extension, ext); From 39b533d854ee2d432f8ba2275b6caa6165e603e4 Mon Sep 17 00:00:00 2001 From: Starbuck5 <46412508+Starbuck5@users.noreply.github.com> Date: Tue, 5 Oct 2021 01:45:30 -0700 Subject: [PATCH 4/6] Update _rwops_from_pystr for dynamic file extension length --- src_c/rwobject.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src_c/rwobject.c b/src_c/rwobject.c index ee6f146aeb..2cd87171f4 100644 --- a/src_c/rwobject.c +++ b/src_c/rwobject.c @@ -741,12 +741,6 @@ _rwops_from_pystr(PyObject *obj) char* ext = NULL; char* extension = NULL; - //longest extension is 4 chars, plus NULL at end - extension = malloc(sizeof(char)*5); - if (extension == NULL) { - return (SDL_RWops*)PyErr_NoMemory(); - } - oencoded = pg_EncodeString(obj, "UTF-8", NULL, NULL); if (oencoded == NULL) { return NULL; @@ -754,9 +748,13 @@ _rwops_from_pystr(PyObject *obj) if (oencoded != Py_None) { encoded = Bytes_AS_STRING(oencoded); rw = SDL_RWFromFile(encoded, "rb"); - ext = strrchr(encoded, '.'); - if (ext) { + ext = strrchr(encoded, '.'); + if (ext && strlen(ext) > 1) { ext++; + extension = malloc(strlen(ext)+1); + if (extension == NULL) { + return (SDL_RWops*)PyErr_NoMemory(); + } strcpy(extension, ext); } } From 3c129c96b9a5f76640410456c358108798fc2821 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Dudfield?= Date: Sat, 16 Oct 2021 10:26:50 +0200 Subject: [PATCH 5/6] test mixer.music: Fixes test since queue raises IOError now --- test/mixer_music_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mixer_music_test.py b/test/mixer_music_test.py index 9ebefc105d..7874566e88 100644 --- a/test/mixer_music_test.py +++ b/test/mixer_music_test.py @@ -167,7 +167,7 @@ def test_queue__invalid_sound_type(self): def test_queue__invalid_filename(self): """Ensures queue() correctly handles invalid filenames.""" - with self.assertRaises(pygame.error): + with self.assertRaises(IOError): pygame.mixer.music.queue("") def todo_test_stop(self): From f207007bfef89b8fecfa399281c5fa4ab1aa4c4f Mon Sep 17 00:00:00 2001 From: Starbuck5 <46412508+Starbuck5@users.noreply.github.com> Date: Sun, 17 Oct 2021 22:28:10 -0700 Subject: [PATCH 6/6] Fix bad merge, API break from pygame.error to FileNotFound --- src_c/music.c | 18 ++++++++++-------- test/mixer_music_test.py | 5 +---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src_c/music.c b/src_c/music.c index 2eeabc022b..69dea0ac54 100644 --- a/src_c/music.c +++ b/src_c/music.c @@ -370,22 +370,24 @@ _get_type_from_hint(char *namehint) } Mix_Music * -_load_music(PyObject *args) { - PyObject *obj; +_load_music(PyObject *obj, char *namehint) { Mix_Music *new_music = NULL; char* ext = NULL; - char *namehint = NULL; SDL_RWops *rw = NULL; - - if (!PyArg_ParseTuple(args, "O|s", &obj, &namehint)) { - return NULL; - } + PyObject* _type = NULL; + PyObject* error = NULL; + PyObject* _traceback = NULL; MIXER_INIT_CHECK(); rw = pgRWops_FromObject(obj); - if (rw == NULL) /* stop on NULL, error already set */ + if (rw == NULL) { /* stop on NULL, error already set is what we SHOULD do */ + PyErr_Fetch(&_type, &error, &_traceback); + PyErr_SetObject(pgExc_SDLError, error); + Py_XDECREF(_type); + Py_XDECREF(_traceback); return NULL; + } if (namehint) { ext = namehint; } else { diff --git a/test/mixer_music_test.py b/test/mixer_music_test.py index 3e378422ec..9254e945e6 100644 --- a/test/mixer_music_test.py +++ b/test/mixer_music_test.py @@ -26,9 +26,6 @@ def setUp(cls): if pygame.mixer.get_init() is None: pygame.mixer.init() - @unittest.skipIf( - "Darwin" in platform.system(), "SDL2_mixer not loading mp3 on travisci" - ) def test_load_mp3(self): "|tags:music|" self.music_load("mp3") @@ -182,7 +179,7 @@ def test_queue__invalid_sound_type(self): def test_queue__invalid_filename(self): """Ensures queue() correctly handles invalid filenames.""" - with self.assertRaises(IOError): + with self.assertRaises(pygame.error): pygame.mixer.music.queue("") def todo_test_stop(self):