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

[pygame] PATCH: movie_set_display hangs on movie from file-like object



Hello world,

I've solved a little problem a had using pygame 1.6 on Win32 Python 2.4.
I initialized a Movie with a file-like object (a cStringIO.StringIO
containing MPEG data), and my script hung when set_display was called on
that Movie.


Note that the current movie module documentation claims that movie won't work on Win32 due to smpeg, and that pygame-1.7.1release.win32-py2.4 emphasizes on this by not containing movie(.pyd). Using pygame 1.7.1 with SDL.dll from SDL 1.2.11 and the current sources of SMPEG and pygame.movie, however, I managed to get it work, and I think, even better than before. (I'll detail that later.)


Now, here's my fix for the function movie_set_display:

Index: movie.c
===================================================================
--- movie.c    (Revision 983)
+++ movie.c    (Working Copy)
@@ -148,32 +148,40 @@

        if(posobj == NULL)
        {
+            Py_BEGIN_ALLOW_THREADS
            SMPEG_Info info;
            SMPEG_getinfo(movie, &info);
            SMPEG_scaleXY(movie, info.width, info.height);
+            Py_END_ALLOW_THREADS
            x = y = 0;
        }
        else if(TwoIntsFromObj(posobj, &x, &y))
        {
+            Py_BEGIN_ALLOW_THREADS
            SMPEG_Info info;
            SMPEG_getinfo(movie, &info);
            SMPEG_scaleXY(movie, info.width, info.height);
+            Py_END_ALLOW_THREADS
        }
        else if((rect = GameRect_FromObject(posobj, &temp)))
        {
            x = rect->x;
            y = rect->y;
+            Py_BEGIN_ALLOW_THREADS
            SMPEG_scaleXY(movie, rect->w, rect->h);
+            Py_END_ALLOW_THREADS
        }
        else
            return RAISE(PyExc_TypeError, "Invalid position argument");

        surf = PySurface_AsSurface(surfobj);

+        Py_BEGIN_ALLOW_THREADS
            SMPEG_getinfo(movie, &info);
        SMPEG_enablevideo(movie, 1);
        SMPEG_setdisplay(movie, surf, NULL, NULL);
        SMPEG_move(movie, x, y);
+        Py_END_ALLOW_THREADS
    }
    else
    {


Reason: When the Movie object is constructed, it creates an SDL_RWops to read the MPEG specified as its parameter. For file names and true file objects, the implementation of SDL_RWops used is based on FILE* and therefore is completely independent of Python.

Since a file-like object is Python data, of course Python's global
interpreter lock must be acquired to access it. The SDL_RWops
implementation used is found in rwobject.c of pygame. See also pygame.h
and movie.c (grep RWopsFromPythonThreaded).

When I called movie_set_display, it reached the call to
SMPEG_getinfo(movie, &info); (see above, near the end of the diff).
Since the call was not preceeded by Py_BEGIN_ALLOW_THREADS, the global
interpreter lock remained on the current thread.

SMPEG_getinfo called the seek function pointed to by the movie's
SDL_RWops, which was rw_seek_th from rwobject.c. rw_seek_th in turn
called PyEval_AcquireLock to protect its access to the file-like object.
PyEval_AcquireLock then deadlocked as documented (the lock had been
acquired twice by the same thread).


I think that my proposed solution is harmless, because - the objects immediately accessed between the inserted Py_{BEGIN,END}_ALLOW_THREADS are not Python data, and - the same practice can already be observed on _any_ other call of an SMPEG_* function in movie.c.


I would also like to suggest some minor changes here:

Index: pygame.h
===================================================================
--- pygame.h    (Revision 983)
+++ pygame.h    (Working Copy)
@@ -56,7 +56,8 @@
     **/
#include <Python.h>

-#ifdef MS_WIN32 /*Python gives us MS_WIN32, SDL needs just WIN32*/
+#if defined(MS_WIN32) && !defined(WIN32)
+/*Python gives us MS_WIN32, SDL needs just WIN32*/
#define WIN32
#endif

@@ -362,6 +363,8 @@
/*last platform compiler stuff*/
#if defined(macintosh) && defined(__MWERKS__)
#define PYGAME_EXPORT __declspec(export)
+#elif defined(MS_WIN32)
+#define PYGAME_EXPORT __declspec(dllexport)
#else
#define PYGAME_EXPORT
#endif


The first change fixes a warning I got about WIN32 being redefined.

The second one should be reviewed. I guess it might not work on every
compiler. I needed it to make pygame.movie and others compile in my
environment (MinGW and Dev-C++, and note that I did not use configure -
maybe that's why).


I promised to tell you what I found improved after using the new versions of SDL and SMPEG: - With the versions from pygame 1.6, I had not been able to scale movies freely. 1x and 2x worked, all else crashed. The new versions fix this. - Also, movies had only been visible on fullscreen surfaces. With SDL 1.2.11, I can finally watch them in a window. - SMPEG up to 0.4.3 muted some audio streams, others played fine. With the current sources, the previously muted audio streams do play, however they are stuttering. Not really an improvement, but a step towards a fix, I think. And the other streams still play fine.

Note that set_volume(0) seems not to work with the new SMPEG (mind the
stuttering). But hey, my speakers have that knob...


I hope I've been helpful, and not too verbose. Please consider re-adding movie.pyd on the next Win32 pygame release.

Bye!

Martin