Fixed two memory-leaks in image handling (ImageFactory and ExternalGraphic). The...

Added ImageIO provider for handling PNG in addition to the internal codec. ImageIO proved to be faster and less memory-intensive for PNGs. ImageIO takes precedence of the internal codec.

git-svn-id: https://svn.apache.org/repos/asf/xmlgraphics/fop/trunk@424349 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/src/java/org/apache/fop/fo/flow/ExternalGraphic.java b/src/java/org/apache/fop/fo/flow/ExternalGraphic.java
index d80cbe8684df1fdbb73f83de8f4cdae0422e6923..a84256b46e2679845c5ac59e069c9a2a197a02b4 100644 (file)
--- a/src/java/org/apache/fop/fo/flow/ExternalGraphic.java
+++ b/src/java/org/apache/fop/fo/flow/ExternalGraphic.java
@@ -41,7 +41,8 @@ public class ExternalGraphic extends AbstractGraphics {
 
     //Additional values
     private String url;
-    private FopImage fopimage;
+    private int intrinsicWidth;
+    private int intrinsicHeight;
     
     /**
      * Create a new External graphic node.
@@ -63,7 +64,7 @@ public class ExternalGraphic extends AbstractGraphics {
         url = ImageFactory.getURL(getSrc());
         FOUserAgent userAgent = getUserAgent();
         ImageFactory fact = userAgent.getFactory().getImageFactory();
-        fopimage = fact.getImage(url, userAgent);
+        FopImage fopimage = fact.getImage(url, userAgent);
         if (fopimage == null) {
             getLogger().error("Image not available: " + getSrc());
         } else {
@@ -71,6 +72,8 @@ public class ExternalGraphic extends AbstractGraphics {
             if (!fopimage.load(FopImage.DIMENSIONS)) {
                 getLogger().error("Cannot read image dimensions: " + getSrc());
             }
+            this.intrinsicWidth = fopimage.getIntrinsicWidth();
+            this.intrinsicHeight = fopimage.getIntrinsicHeight();
         }
         //TODO Report to caller so he can decide to throw an exception
     }
@@ -122,22 +125,14 @@ public class ExternalGraphic extends AbstractGraphics {
      * @see org.apache.fop.fo.flow.AbstractGraphics#getIntrinsicWidth()
      */
     public int getIntrinsicWidth() {
-        if (fopimage != null) {
-            return fopimage.getIntrinsicWidth();
-        } else {
-            return 0;
-        }
+        return this.intrinsicWidth;
     }
 
     /**
      * @see org.apache.fop.fo.flow.AbstractGraphics#getIntrinsicHeight()
      */
     public int getIntrinsicHeight() {
-        if (fopimage != null) {
-            return fopimage.getIntrinsicHeight();
-        } else {
-            return 0;
-        }
+        return this.intrinsicHeight;
     }
     
 }

diff --git a/src/java/org/apache/fop/image/ImageFactory.java b/src/java/org/apache/fop/image/ImageFactory.java
index 376dc6686e3717cfc8ad50b3edac1c95adb93632..acf310ada6e67b26cced6a60b3b74cdef8e4b7ad 100644 (file)
--- a/src/java/org/apache/fop/image/ImageFactory.java
+++ b/src/java/org/apache/fop/image/ImageFactory.java
@@ -20,6 +20,9 @@ package org.apache.fop.image;
 
 // Java
 import java.io.InputStream;
+import java.lang.ref.Reference;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.SoftReference;
 import java.lang.reflect.Constructor;
 import java.util.ArrayList;
 import java.util.Map;
@@ -28,6 +31,8 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map.Entry;
+
 import javax.xml.transform.Source;
 import javax.xml.transform.stream.StreamSource;
 
@@ -39,7 +44,6 @@ import org.apache.fop.image.analyser.ImageReaderFactory;
 import org.apache.fop.apps.FOUserAgent;
 import org.apache.fop.datatypes.URISpecification;
 
-
 /**
  * Create FopImage objects (with a configuration file - not yet implemented).
  * @author Eric SCHAEFFER
@@ -99,6 +103,8 @@ public final class ImageFactory {
 
         imt = new ImageMimeType("image/png");
         imageMimeTypes.put(imt.getMimeType(), imt);
+        //Image I/O is faster and more memory-efficient than own codec for PNG
+        imt.addProvider(imageIoImage);
         imt.addProvider(pngImage);
 
         imt = new ImageMimeType("image/tga");
@@ -109,8 +115,9 @@ public final class ImageFactory {
 
         imt = new ImageMimeType("image/tiff");
         imageMimeTypes.put(imt.getMimeType(), imt);
-        imt.addProvider(tiffImage);
-        imt.addProvider(jaiImage);
+        imt.addProvider(tiffImage); //Slower but supports CCITT embedding
+        imt.addProvider(imageIoImage); //Fast but doesn't support CCITT embedding
+        imt.addProvider(jaiImage); //Fast but doesn't support CCITT embedding
 
         imt = new ImageMimeType("image/svg+xml");
         imageMimeTypes.put(imt.getMimeType(), imt);
@@ -357,12 +364,13 @@ class ContextImageCache implements ImageCache {
     private boolean collective;
     private Map contextStore = Collections.synchronizedMap(new java.util.HashMap());
     private Set invalid = null;
-    private Map weakStore = null;
+    private Map refStore = null;
+    private ReferenceQueue refQueue = new ReferenceQueue();
 
     public ContextImageCache(boolean col) {
         collective = col;
         if (collective) {
-            weakStore = Collections.synchronizedMap(new java.util.WeakHashMap());
+            refStore = Collections.synchronizedMap(new java.util.HashMap());
             invalid = Collections.synchronizedSet(new java.util.HashSet());
         }
     }
@@ -399,7 +407,14 @@ class ContextImageCache implements ImageCache {
                     }
                 }
                 if (im == null) {
-                    im = (ImageLoader) weakStore.get(url);
+                    Reference ref = (Reference)refStore.get(url);
+                    if (ref != null) {
+                        im = (ImageLoader) ref.get();
+                        if (im == null) {
+                            //Remove key if its value has been garbage collected
+                            refStore.remove(url);
+                        }
+                    }
                 }
             }
 
@@ -423,7 +438,7 @@ class ContextImageCache implements ImageCache {
         if (con != null) {
             if (collective) {
                 ImageLoader im = con.getImage(url);
-                weakStore.put(url, im);
+                refStore.put(url, wrapInReference(im, url));
             }
             con.releaseImage(url);
         }
@@ -443,15 +458,47 @@ class ContextImageCache implements ImageCache {
         }
     }
 
+    private Reference wrapInReference(Object obj, Object key) {
+        return new SoftReferenceWithKey(obj, key, refQueue);
+    }
+    
+    private static class SoftReferenceWithKey extends SoftReference {
+        
+        private Object key;
+        
+        public SoftReferenceWithKey(Object referent, Object key, ReferenceQueue q) {
+            super(referent, q);
+            this.key = key;
+        }
+    }
+    
     public void removeContext(FOUserAgent context) {
         Context con = (Context) contextStore.get(context);
         if (con != null) {
             if (collective) {
                 Map images = con.getImages();
-                weakStore.putAll(images);
+                Iterator iter = images.entrySet().iterator();
+                while (iter.hasNext()) {
+                    Entry entry = (Entry)iter.next();
+                    refStore.put(entry.getKey(), 
+                            wrapInReference(entry.getValue(), entry.getKey()));
+                }
             }
             contextStore.remove(context);
         }
+        //House-keeping (remove cleared references)
+        checkReferenceQueue();
+    }
+    
+    /**
+     * Checks the reference queue if any references have been cleared and removes them from the
+     * cache.
+     */
+    private void checkReferenceQueue() {
+        SoftReferenceWithKey ref;
+        while ((ref = (SoftReferenceWithKey)refQueue.poll()) != null) {
+            refStore.remove(ref.key);
+        }
     }
 
     class Context {
@@ -503,7 +550,7 @@ class ContextImageCache implements ImageCache {
 
     /** @see org.apache.fop.image.ImageCache#clearAll() */
     public void clearAll() {
-        this.weakStore.clear();
+        this.refStore.clear();
         this.invalid.clear();
         //The context-sensitive caches are not cleared so there are no negative side-effects
         //in a multi-threaded environment. Not that it's a good idea to use this method at

diff --git a/status.xml b/status.xml
index 00d1d5470a9448f06805d768cafd6248410e9266..c7f3a405405f9d6eb32b06d531a7f7201024a311 100644 (file)
--- a/status.xml
+++ b/status.xml
@@ -27,6 +27,15 @@
 
   <changes>
     <release version="FOP Trunk">
+      <action context="Code" dev="JM" type="fix">
+        Fixed two memory-leaks in image handling. The image cache is finally working
+        properly.
+      </action>
+      <action context="Code" dev="JM" type="fix" fixes-bug="39608">
+        Let numeric property values without a unit be treated as pixels like in HTML.
+        This fixes certain NullPointerException when no units are specified.
+        (Note: the use of pixels in XSL-FO is discouraged!)
+      </action>
       <action context="Code" dev="JM" type="fix">
         Bugfix: Potential multi-threading issue (ConcurrentModificationException) 
         eliminated for ElementMapping classes.