Hi, While trying to get WW2D work out of the box (http://ww2d.csoft.net/ which is really cool!) I found a nasty locking problem between the main gdk lock (which is acquired by jogl through our jawt lock gdk_threads_enter implementation) and the gdkpixbuf functions we use to load/create images in GtkImage and GdkPixbufDecoder. This is a problem since the main gdk lock isn't reentrant and WW2D (can) load images while holding the jawt->lock().
Since it seems acquiring the main gdk lock isn't really necessary for
all pixbuf operations I added a new lock to guard all gdkpixbuf
operations. Only for those functions that manipulate Graphics state also
the main gdk lock is acquired. That makes all image loading operations
synchronized, but not depend on the main gdk lock, so they can be done
in parallel to actual widget/drawing manipulation. Things that don't
change gdk state seem fair game here since the main GLib operations are
thread safe already. And by having a global gdkpixbuf lock we make sure
any gdkpixbuf internal state is only manipulated from one thread at a
time.
2006-03-19 Mark Wielaard <[EMAIL PROTECTED]>
* gnu/java/awt/peer/gtk/GdkPixbufDecoder.java (pixbufLock): New
static lock Object field.
(produce): Synchronize on pixbufLock when calling initState(),
pumpBytes() and pumpDone().
(finalize): Likewise when calling finish().
(GdkPixbufWriter.write): Likewise when calling streamImage().
* gnu/java/awt/peer/gtk/GtkImage.java (GtkImage(String)): Likewise
when calling loadPixbuf. Chain exception.
(GtkImage(byte[])): Likewise when calling loadImageFromData.
(GtkImage(URL)): Likewise.
(GtkImage(int,int)): Likewise when calling createPixmap().
(GtkImage(GtkImage,int,int,int)): Likewise when calling
createScaledPixmap().
(GtkImage(Pointer)): Likewise when calling createFromPixbuf().
(setImage): Likewise when calling createPixmap() and setPixels().
(getSource): Likewise when calling getPixels().
(flush): Likewise when calling freePixmap().
(finalize): Likewise.
(drawImage): Likewise when calling drawPixelsScaledFlipped() and
drawPixelsScaledFlipped().
* native/jni/gtk-peer/gnu_java_awt_peer_gtk_GdkPixbufDecoder.c
(Java_gnu_java_awt_peer_gtk_GdkPixbufDecoder_initState): Remove
gdk_threads_enter/leave().
(Java_gnu_java_awt_peer_gtk_GdkPixbufDecoder_finish): Likewise.
(Java_gnu_java_awt_peer_gtk_GdkPixbufDecoder_pumpDone): Likewise.
(Java_gnu_java_awt_peer_gtk_GdkPixbufDecoder_streamImage): Likewise.
(Java_gnu_java_awt_peer_gtk_GdkPixbufDecoder_pumpBytes): Likewise.
* native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkImage.c
(Java_gnu_java_awt_peer_gtk_GtkImage_loadPixbuf): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_loadImageFromData): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_createFromPixbuf): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_getPixels): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_setPixels): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_createPixmap): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_freePixmap): Likewise.
(Java_gnu_java_awt_peer_gtk_GtkImage_createScaledPixmap): Likewise.
Comments?
This setup looks safe to me and makes WW2D work out of the box. (At
least with the mouse, you can zoom in and out with the scroll wheel now.
I still need to look into why KeyEvents aren't usable).
I added a FIXME comment to save_to_stream(). There we drop the main gdk
lock explicitly and reacquire it after calling user code. We really
shouldn't do that. It would be better to quickly safe the bytes and then
have a separate thread that "pumps" the bytes to the user InputStream
code for processing. Then we don't need to play tricks with the locking.
Cheers,
Mark
Index: gnu/java/awt/peer/gtk/GdkPixbufDecoder.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/awt/peer/gtk/GdkPixbufDecoder.java,v
retrieving revision 1.18
diff -u -r1.18 GdkPixbufDecoder.java
--- gnu/java/awt/peer/gtk/GdkPixbufDecoder.java 21 Nov 2005 22:46:52 -0000 1.18
+++ gnu/java/awt/peer/gtk/GdkPixbufDecoder.java 19 Mar 2006 10:16:18 -0000
@@ -1,5 +1,5 @@
/* GdkPixbufDecoder.java -- Image data decoding object
- Copyright (C) 2003, 2004, 2005 Free Software Foundation, Inc.
+ Copyright (C) 2003, 2004, 2005, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -82,6 +82,14 @@
initStaticState ();
}
+ /**
+ * Lock that should be held for all gdkpixbuf operations. We don't use
+ * the global gdk_threads_enter/leave functions since gdkpixbuf
+ * operations can be done in parallel to drawing and manipulating gtk
+ * widgets.
+ */
+ static Object pixbufLock = new Object();
+
static native void initStaticState();
private final int native_state = GtkGenericPeer.getUniqueInteger ();
@@ -92,6 +100,7 @@
Vector curr;
// interface to GdkPixbuf
+ // These native functions should be called with the pixbufLock held.
native void initState ();
native void pumpBytes (byte[] bytes, int len) throws IOException;
native void pumpDone () throws IOException;
@@ -171,11 +180,26 @@
byte bytes[] = new byte[4096];
int len = 0;
- initState();
+ synchronized(pixbufLock)
+ {
+ initState();
+ }
needsClose = true;
+
+ // Note: We don't want the pixbufLock while reading from the InputStream.
while ((len = is.read (bytes)) != -1)
- pumpBytes (bytes, len);
- pumpDone();
+ {
+ synchronized(pixbufLock)
+ {
+ pumpBytes (bytes, len);
+ }
+ }
+
+ synchronized(pixbufLock)
+ {
+ pumpDone();
+ }
+
needsClose = false;
for (int i = 0; i < curr.size (); i++)
@@ -189,7 +213,10 @@
public void finalize()
{
- finish(needsClose);
+ synchronized(pixbufLock)
+ {
+ finish(needsClose);
+ }
}
@@ -495,8 +522,11 @@
}
processImageStarted(1);
- streamImage(pixels, this.ext, width, height, model.hasAlpha(),
- (DataOutput) this.getOutput());
+ synchronized(pixbufLock)
+ {
+ streamImage(pixels, this.ext, width, height, model.hasAlpha(),
+ (DataOutput) this.getOutput());
+ }
processImageComplete();
}
}
Index: gnu/java/awt/peer/gtk/GtkImage.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/awt/peer/gtk/GtkImage.java,v
retrieving revision 1.26
diff -u -r1.26 GtkImage.java
--- gnu/java/awt/peer/gtk/GtkImage.java 14 Feb 2006 22:49:17 -0000 1.26
+++ gnu/java/awt/peer/gtk/GtkImage.java 19 Mar 2006 10:16:18 -0000
@@ -123,41 +123,50 @@
/**
* Returns a copy of the pixel data as a java array.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native int[] getPixels();
/**
* Sets the pixel data from a java array.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native void setPixels(int[] pixels);
/**
* Loads an image using gdk-pixbuf from a file.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native boolean loadPixbuf(String name);
/**
* Loads an image using gdk-pixbuf from data.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native boolean loadImageFromData(byte[] data);
/**
* Allocates a Gtk Pixbuf or pixmap
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native void createPixmap();
/**
* Frees the above.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native void freePixmap();
/**
* Sets the pixmap to scaled copy of src image. hints are rendering hints.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native void createScaledPixmap(GtkImage src, int hints);
/**
* Draws the image, optionally scaled and composited.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
+ * Also acquires global gdk lock for drawing.
*/
private native void drawPixelsScaled (GdkGraphics gc,
int bg_red, int bg_green, int bg_blue,
@@ -166,6 +175,8 @@
/**
* Draws the image, optionally scaled flipped and composited.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
+ * Also acquires global gdk lock for drawing.
*/
private native void drawPixelsScaledFlipped (GdkGraphics gc,
int bg_red, int bg_green,
@@ -219,12 +230,21 @@
File f = new File(filename);
try
{
- if (loadPixbuf(f.getCanonicalPath()) != true)
- throw new IllegalArgumentException("Couldn't load image: "+filename);
+ String path = f.getCanonicalPath();
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ if (loadPixbuf(f.getCanonicalPath()) != true)
+ throw new IllegalArgumentException("Couldn't load image: "
+ + filename);
+ }
}
catch(IOException e)
{
- throw new IllegalArgumentException("Couldn't load image: "+filename);
+ IllegalArgumentException iae;
+ iae = new IllegalArgumentException("Couldn't load image: "
+ + filename);
+ iae.initCause(e);
+ throw iae;
}
isLoaded = true;
@@ -241,8 +261,11 @@
*/
public GtkImage (byte[] data)
{
- if (loadImageFromData (data) != true)
- throw new IllegalArgumentException ("Couldn't load image.");
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ if (loadImageFromData (data) != true)
+ throw new IllegalArgumentException ("Couldn't load image.");
+ }
isLoaded = true;
observers = null;
@@ -277,8 +300,12 @@
{
throw new IllegalArgumentException ("Couldn't load image.");
}
- if (loadImageFromData (baos.toByteArray()) != true)
- throw new IllegalArgumentException ("Couldn't load image.");
+ byte[] array = baos.toByteArray();
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ if (loadImageFromData(array) != true)
+ throw new IllegalArgumentException ("Couldn't load image.");
+ }
isLoaded = true;
observers = null;
@@ -296,7 +323,10 @@
isLoaded = true;
observers = null;
offScreen = true;
- createPixmap();
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ createPixmap();
+ }
}
/**
@@ -312,7 +342,10 @@
offScreen = false;
// Use the GDK scaling method.
- createScaledPixmap(src, hints);
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ createScaledPixmap(src, hints);
+ }
}
/**
@@ -322,7 +355,10 @@
GtkImage (Pointer pixbuf)
{
pixmap = pixbuf;
- createFromPixbuf();
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ createFromPixbuf();
+ }
isLoaded = true;
observers = null;
offScreen = false;
@@ -349,6 +385,7 @@
/**
* Native helper function for constructor that takes a pixbuf Pointer.
+ * Should be called with the GdkPixbufDecoder.pixbufLock held.
*/
private native void createFromPixbuf();
@@ -370,8 +407,11 @@
isLoaded = true;
deliver();
- createPixmap();
- setPixels(pixels);
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ createPixmap();
+ setPixels(pixels);
+ }
}
// java.awt.Image methods ////////////////////////////////////////////////
@@ -408,7 +448,13 @@
{
if (!isLoaded)
return null;
- return new MemoryImageSource(width, height, nativeModel, getPixels(),
+
+ int[] pixels;
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ pixels = getPixels();
+ }
+ return new MemoryImageSource(width, height, nativeModel, pixels,
0, width);
}
@@ -454,7 +500,10 @@
{
observers = new Vector();
isLoaded = false;
- freePixmap();
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ freePixmap();
+ }
source.startProduction(new GtkImageConsumer(this, source));
}
}
@@ -462,7 +511,12 @@
public void finalize()
{
if (isLoaded)
- freePixmap();
+ {
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ freePixmap();
+ }
+ }
}
/**
@@ -532,20 +586,23 @@
if ( srcWidth <= 0 || srcHeight <= 0 || dstWidth <= 0 || dstHeight <= 0)
return true;
- if(bgcolor != null)
- drawPixelsScaledFlipped (g, bgcolor.getRed (), bgcolor.getGreen (),
- bgcolor.getBlue (),
- flipX, flipY,
- srcX, srcY,
- srcWidth, srcHeight,
- dstX, dstY,
- dstWidth, dstHeight,
- true);
- else
- drawPixelsScaledFlipped (g, 0, 0, 0, flipX, flipY,
- srcX, srcY, srcWidth, srcHeight,
- dstX, dstY, dstWidth, dstHeight,
- false);
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ if(bgcolor != null)
+ drawPixelsScaledFlipped (g, bgcolor.getRed (), bgcolor.getGreen (),
+ bgcolor.getBlue (),
+ flipX, flipY,
+ srcX, srcY,
+ srcWidth, srcHeight,
+ dstX, dstY,
+ dstWidth, dstHeight,
+ true);
+ else
+ drawPixelsScaledFlipped (g, 0, 0, 0, flipX, flipY,
+ srcX, srcY, srcWidth, srcHeight,
+ dstX, dstY, dstWidth, dstHeight,
+ false);
+ }
return true;
}
@@ -559,11 +616,14 @@
if (addObserver(observer))
return false;
- if(bgcolor != null)
- drawPixelsScaled(g, bgcolor.getRed (), bgcolor.getGreen (),
- bgcolor.getBlue (), x, y, width, height, true);
- else
- drawPixelsScaled(g, 0, 0, 0, x, y, width, height, false);
+ synchronized(GdkPixbufDecoder.pixbufLock)
+ {
+ if(bgcolor != null)
+ drawPixelsScaled(g, bgcolor.getRed (), bgcolor.getGreen (),
+ bgcolor.getBlue (), x, y, width, height, true);
+ else
+ drawPixelsScaled(g, 0, 0, 0, x, y, width, height, false);
+ }
return true;
}
Index: native/jni/gtk-peer/gnu_java_awt_peer_gtk_GdkPixbufDecoder.c
===================================================================
RCS file: /cvsroot/classpath/classpath/native/jni/gtk-peer/gnu_java_awt_peer_gtk_GdkPixbufDecoder.c,v
retrieving revision 1.21
diff -u -r1.21 gnu_java_awt_peer_gtk_GdkPixbufDecoder.c
--- native/jni/gtk-peer/gnu_java_awt_peer_gtk_GdkPixbufDecoder.c 25 Nov 2005 09:45:38 -0000 1.21
+++ native/jni/gtk-peer/gnu_java_awt_peer_gtk_GdkPixbufDecoder.c 19 Mar 2006 10:16:18 -0000
@@ -1,5 +1,5 @@
/* gdkpixbufdecoder.c
- Copyright (C) 1999, 2003, 2004, 2005 Free Software Foundation, Inc.
+ Copyright (C) 1999, 2003, 2004, 2005, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -191,8 +191,6 @@
GdkPixbufLoader *loader = NULL;
jobject *decoder = NULL;
- gdk_threads_enter ();
-
decoder = (jobject *) g_malloc (sizeof (jobject));
g_assert (decoder != NULL);
*decoder = (*env)->NewGlobalRef (env, obj);
@@ -204,8 +202,6 @@
g_signal_connect (loader, "closed", G_CALLBACK (closed_cb), decoder);
NSA_SET_PB_PTR (env, obj, loader);
-
- gdk_threads_leave ();
}
static void
@@ -317,8 +313,6 @@
{
GdkPixbufLoader *loader = NULL;
- gdk_threads_enter ();
-
loader = (GdkPixbufLoader *)NSA_DEL_PB_PTR (env, obj);
if (loader == NULL)
return;
@@ -326,8 +320,6 @@
if (needs_close)
gdk_pixbuf_loader_close (loader, NULL);
g_object_unref (loader);
-
- gdk_threads_leave ();
}
JNIEXPORT void JNICALL
@@ -337,8 +329,6 @@
GError *err = NULL;
GdkPixbufLoader *loader = NULL;
- gdk_threads_enter ();
-
loader = (GdkPixbufLoader *)NSA_GET_PB_PTR (env, obj);
g_assert (loader != NULL);
@@ -349,8 +339,6 @@
JCL_ThrowException (env, "java/io/IOException", err->message);
g_error_free (err);
}
-
- gdk_threads_leave ();
}
struct stream_save_request
@@ -370,6 +358,9 @@
jbyteArray jbuf;
jbyte *cbuf;
+ /* FIXME. Don't call user code directly on this thread.
+ Store bytes and signal a "pump" thread to deliver to user code.
+ Then we don't have to drop/acquire any locks. */
gdk_threads_leave ();
jbuf = (*(ssr->env))->NewByteArray ((ssr->env), count);
@@ -400,8 +391,6 @@
int i;
struct stream_save_request ssr;
- gdk_threads_enter ();
-
ssr.stream = &stream;
ssr.env = env;
@@ -465,8 +454,6 @@
(*env)->ReleaseStringUTFChars (env, jenctype, enctype);
(*env)->ReleaseIntArrayElements (env, jarr, ints, 0);
-
- gdk_threads_leave ();
}
@@ -478,8 +465,6 @@
jbyte *bytes = NULL;
GError *err = NULL;
- gdk_threads_enter ();
-
g_assert (len >= 1);
g_assert (jarr != NULL);
@@ -497,6 +482,4 @@
JCL_ThrowException (env, "java/io/IOException", err->message);
g_error_free (err);
}
-
- gdk_threads_leave ();
}
Index: native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkImage.c
===================================================================
RCS file: /cvsroot/classpath/classpath/native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkImage.c,v
retrieving revision 1.17
diff -u -r1.17 gnu_java_awt_peer_gtk_GtkImage.c
--- native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkImage.c 22 Sep 2005 20:25:39 -0000 1.17
+++ native/jni/gtk-peer/gnu_java_awt_peer_gtk_GtkImage.c 19 Mar 2006 10:16:18 -0000
@@ -1,5 +1,5 @@
/* gtkimage.c
- Copyright (C) 2005 Free Software Foundation, Inc.
+ Copyright (C) 2005, 2006 Free Software Foundation, Inc.
This file is part of GNU Classpath.
@@ -65,23 +65,17 @@
int width, height;
GdkPixbuf *pixbuf;
- gdk_threads_enter ();
-
/* Don't use the JCL convert function because it throws an exception
on failure */
filename = (*env)->GetStringUTFChars (env, name, 0);
if (filename == NULL)
- {
- gdk_threads_leave ();
- return JNI_FALSE;
- }
+ return JNI_FALSE;
pixbuf = gdk_pixbuf_new_from_file (filename, NULL);
if (pixbuf == NULL)
{
(*env)->ReleaseStringUTFChars (env, name, filename);
- gdk_threads_leave ();
return JNI_FALSE;
}
@@ -92,8 +86,6 @@
setWidthHeight(env, obj, width, height);
(*env)->ReleaseStringUTFChars (env, name, filename);
- gdk_threads_leave ();
-
return JNI_TRUE;
}
@@ -111,8 +103,6 @@
int width;
int height;
- gdk_threads_enter ();
-
src = (*env)->GetByteArrayElements (env, data, NULL);
len = (*env)->GetArrayLength (env, data);
@@ -128,9 +118,6 @@
if (pixbuf == NULL)
{
createRawData (env, obj, NULL);
-
- gdk_threads_leave ();
-
return JNI_FALSE;
}
@@ -140,8 +127,6 @@
createRawData (env, obj, pixbuf);
setWidthHeight(env, obj, width, height);
- gdk_threads_leave ();
-
return JNI_TRUE;
}
@@ -151,10 +136,8 @@
{
int width, heigth;
GdkPixbuf *pixbuf = (GdkPixbuf *) getData (env, obj);
- gdk_threads_enter ();
width = gdk_pixbuf_get_width (pixbuf);
heigth = gdk_pixbuf_get_height (pixbuf);
- gdk_threads_leave ();
setWidthHeight(env, obj, width, heigth);
}
@@ -171,8 +154,6 @@
jint *result_array_iter, *dst;
int i,j;
- gdk_threads_enter ();
-
pixbuf = cp_gtk_image_get_pixbuf (env, obj);
width = gdk_pixbuf_get_width (pixbuf);
height = gdk_pixbuf_get_height (pixbuf);
@@ -214,8 +195,6 @@
(*env)->ReleaseIntArrayElements (env, result_array, result_array_iter, 0);
- gdk_threads_leave ();
-
return result_array;
}
@@ -233,8 +212,6 @@
jint *src_array_iter, *src;
int i;
- gdk_threads_enter ();
-
width = gdk_pixbuf_get_width (pixbuf);
height = gdk_pixbuf_get_height (pixbuf);
rowstride = gdk_pixbuf_get_rowstride (pixbuf);
@@ -251,8 +228,6 @@
}
(*env)->ReleaseIntArrayElements (env, pixels, src_array_iter, 0);
-
- gdk_threads_leave ();
}
/**
@@ -265,8 +240,6 @@
jclass cls;
jfieldID field;
- gdk_threads_enter ();
-
cls = (*env)->GetObjectClass (env, obj);
field = (*env)->GetFieldID (env, cls, "width", "I");
g_assert (field != 0);
@@ -285,8 +258,6 @@
else
createRawData (env, obj, gdk_pixmap_new (NULL, width, height,
gdk_rgb_get_visual ()->depth));
-
- gdk_threads_leave ();
}
/**
@@ -295,13 +266,10 @@
JNIEXPORT void JNICALL
Java_gnu_java_awt_peer_gtk_GtkImage_freePixmap(JNIEnv *env, jobject obj)
{
- gdk_threads_enter ();
if (offScreen (env, obj) == JNI_FALSE)
gdk_pixbuf_unref ((GdkPixbuf *)getData (env, obj));
else
g_object_unref ((GdkPixmap *)getData (env, obj));
-
- gdk_threads_leave ();
}
/**
@@ -321,8 +289,6 @@
GdkPixbuf *pixbuf;
- gdk_threads_enter ();
-
cls = (*env)->GetObjectClass (env, destination);
field = (*env)->GetFieldID (env, cls, "width", "I");
g_assert (field != 0);
@@ -342,8 +308,6 @@
gdk_pixbuf_unref (pixbuf);
createRawData (env, destination, (void *)dst);
-
- gdk_threads_leave ();
}
/**
signature.asc
Description: This is a digitally signed message part
