Hi,
The attached patch fixes the failing mauve tests for ConvolveOp, and
also touches up the ColorConvertOp to use the correct transfer type.
Any comments (or objections) before committing?
Thanks,
Francis
2006-08-24 Francis Kung <[EMAIL PROTECTED]>
* java/awt/image/ColorConvertOp.java
(copyImage): Updated javadoc and comments.
(copyRaster): Add javadoc.
(createCompatibleColorModel): Add javadocs and comments.
(createCompatibleDestImage): Use correct transfer type.
(createCompatibleDestRaster): Add new parameter for transfer type.
(filter): Use correct transfer type.
* java/awt/image/ConvolveOp.java: Updated javadocs.
(createCompatibleDestImage): Set new image properties correctly.
(filter(BufferedImage, BufferedImage): Correct handling of
premultiplication.
(filter(WritableRaster, Raster): Clip sample values to [0-255].
Index: java/awt/image/ColorConvertOp.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/awt/image/ColorConvertOp.java,v
retrieving revision 1.6
diff -u -r1.6 ColorConvertOp.java
--- java/awt/image/ColorConvertOp.java 24 Aug 2006 15:49:55 -0000 1.6
+++ java/awt/image/ColorConvertOp.java 24 Aug 2006 20:42:21 -0000
@@ -38,6 +38,8 @@
package java.awt.image;
+import gnu.java.awt.Buffers;
+
import java.awt.Graphics2D;
import java.awt.Point;
import java.awt.RenderingHints;
@@ -283,7 +285,8 @@
for (int i = 0; i < spaces.length - 2; i++)
{
WritableRaster tmp = createCompatibleDestRaster(src, spaces[i + 1],
- false);
+ false,
+ src.getTransferType());
copyraster(src, spaces[i], tmp, spaces[i + 1]);
src = tmp;
}
@@ -291,7 +294,8 @@
// The last conversion is done outside of the loop so that we can
// use the dest raster supplied, instead of creating our own temp raster
if (dest == null)
- dest = createCompatibleDestRaster(src, spaces[spaces.length - 1], false);
+ dest = createCompatibleDestRaster(src, spaces[spaces.length - 1], false,
+ DataBuffer.TYPE_BYTE);
copyraster(src, spaces[spaces.length - 2], dest, spaces[spaces.length - 1]);
return dest;
@@ -324,7 +328,8 @@
return new BufferedImage(dstCM,
createCompatibleDestRaster(src.getRaster(),
dstCM.getColorSpace(),
- src.getColorModel().hasAlpha),
+ src.getColorModel().hasAlpha,
+ dstCM.getTransferType()),
src.isPremultiplied, null);
}
@@ -349,7 +354,8 @@
// Create a new raster with the last ColorSpace in the conversion
// chain, and with no alpha (implied)
- return createCompatibleDestRaster(src, spaces[spaces.length-1], false);
+ return createCompatibleDestRaster(src, spaces[spaces.length-1], false,
+ DataBuffer.TYPE_BYTE);
}
/**
@@ -417,11 +423,16 @@
return src.getBounds();
}
- // Copy a source image to a destination image, respecting their colorspaces
- // and performing colorspace conversions if necessary. This is done
- // using Graphics2D in order to use the rendering hints.
+ /**
+ * Copy a source image to a destination image, respecting their colorspaces
+ * and performing colorspace conversions if necessary.
+ *
+ * @param src The source image.
+ * @param dst The destination image.
+ */
private void copyimage(BufferedImage src, BufferedImage dst)
{
+ // This is done using Graphics2D in order to respect the rendering hints.
Graphics2D gg = dst.createGraphics();
// If no hints are set there is no need to call
@@ -433,8 +444,16 @@
gg.dispose();
}
- // Copy a source raster to a destination raster, performing a colorspace
- // conversion.
+ /**
+ * Copy a source raster to a destination raster, performing a colorspace
+ * conversion between the two. The conversion will respect the
+ * KEY_COLOR_RENDERING rendering hint if one is present.
+ *
+ * @param src The source raster.
+ * @param scs The colorspace of the source raster.
+ * @dst The destination raster.
+ * @dcs The colorspace of the destination raster.
+ */
private void copyraster(Raster src, ColorSpace scs, WritableRaster dst, ColorSpace dcs)
{
float[] sbuf = new float[src.getNumBands()];
@@ -459,11 +478,19 @@
}
}
- // This method creates a compatible color model, given a source image and
- // a colorspace. The choice of ComponentColorModel and DataBuffer.TYPE_BYTE
- // is based on Mauve testing of the reference implementation.
+ /**
+ * This method creates a color model with the same colorspace and alpha
+ * settings as the source image. The created color model will always be a
+ * ComponentColorModel and have a BYTE transfer type.
+ *
+ * @param img The source image.
+ * @param cs The ColorSpace to use.
+ * @return A color model compatible with the source image.
+ */
private ColorModel createCompatibleColorModel(BufferedImage img, ColorSpace cs)
{
+ // The choice of ComponentColorModel and DataBuffer.TYPE_BYTE is based on
+ // Mauve testing of the reference implementation.
return new ComponentColorModel(cs,
img.getColorModel().hasAlpha(),
img.isAlphaPremultiplied(),
@@ -471,13 +498,22 @@
DataBuffer.TYPE_BYTE);
}
- // This method creates a compatible Raster, given a source raster and
- // colorspace.
- private WritableRaster createCompatibleDestRaster(Raster src, ColorSpace cs, boolean hasAlpha)
- {
- // The use of a PixelInterleavedSampleModel (and it's parameters) and
- // a DataBufferByte were determined using mauve tests, based on the
- // reference implementation
+ /**
+ * This method creates a compatible Raster, given a source raster, colorspace,
+ * alpha value, and transfer type.
+ *
+ * @param src The source raster.
+ * @param cs The ColorSpace to use.
+ * @param hasAlpha Whether the raster should include a component for an alpha.
+ * @param transferType The size of a single data element.
+ * @return A compatible WritableRaster.
+ */
+ private WritableRaster createCompatibleDestRaster(Raster src, ColorSpace cs,
+ boolean hasAlpha,
+ int transferType)
+ {
+ // The use of a PixelInterleavedSampleModel weas determined using mauve
+ // tests, based on the reference implementation
int numComponents = cs.getNumComponents();
if (hasAlpha)
@@ -487,13 +523,15 @@
for (int i = 0; i < offsets.length; i++)
offsets[i] = i;
- return new WritableRaster(new PixelInterleavedSampleModel(DataBuffer.TYPE_BYTE,
+ DataBuffer db = Buffers.createBuffer(transferType,
+ src.getWidth() * src.getHeight() * numComponents,
+ 1);
+ return new WritableRaster(new PixelInterleavedSampleModel(transferType,
src.getWidth(),
src.getHeight(),
numComponents,
numComponents * src.getWidth(),
offsets),
- new DataBufferByte(src.getWidth() * src.getHeight() * numComponents, 1),
- new Point(src.getMinX(), src.getMinY()));
+ db, new Point(src.getMinX(), src.getMinY()));
}
}
Index: java/awt/image/ConvolveOp.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/awt/image/ConvolveOp.java,v
retrieving revision 1.9
diff -u -r1.9 ConvolveOp.java
--- java/awt/image/ConvolveOp.java 20 Jul 2006 08:28:53 -0000 1.9
+++ java/awt/image/ConvolveOp.java 24 Aug 2006 20:42:21 -0000
@@ -38,7 +38,6 @@
package java.awt.image;
-import java.awt.Graphics2D;
import java.awt.RenderingHints;
import java.awt.geom.Point2D;
import java.awt.geom.Rectangle2D;
@@ -51,11 +50,13 @@
* with elements in the kernel to compute a new pixel.
*
* Each band in a Raster is convolved and copied to the destination Raster.
+ * For BufferedImages, convolution is applied to all components. Color
+ * conversion will be applied if needed.
*
- * For BufferedImages, convolution is applied to all components. If the
- * source is not premultiplied, the data will be premultiplied before
- * convolving. Premultiplication will be undone if the destination is not
- * premultiplied. Color conversion will be applied if needed.
+ * Note that this filter ignores whether the source or destination is alpha
+ * premultiplied. The reference spec states that data will be premultiplied
+ * prior to convolving and divided back out afterwards (if needed), but testing
+ * has shown that this is not the case with their implementation.
*
* @author [EMAIL PROTECTED]
*/
@@ -104,59 +105,83 @@
hints = null;
}
-
- /* (non-Javadoc)
- * @see java.awt.image.BufferedImageOp#filter(java.awt.image.BufferedImage,
- * java.awt.image.BufferedImage)
+ /**
+ * Converts the source image using the kernel specified in the
+ * constructor. The resulting image is stored in the destination image if one
+ * is provided; otherwise a new BufferedImage is created and returned.
+ *
+ * The source and destination BufferedImage (if one is supplied) must have
+ * the same dimensions.
+ *
+ * @param src The source image.
+ * @param dst The destination image.
+ * @throws IllegalArgumentException if the rasters and/or color spaces are
+ * incompatible.
+ * @return The convolved image.
*/
public final BufferedImage filter(BufferedImage src, BufferedImage dst)
{
if (src == dst)
- throw new IllegalArgumentException();
+ throw new IllegalArgumentException("Source and destination images " +
+ "cannot be the same.");
if (dst == null)
dst = createCompatibleDestImage(src, src.getColorModel());
// Make sure source image is premultiplied
BufferedImage src1 = src;
- if (!src.isPremultiplied)
+ // The spec says we should do this, but mauve testing shows that Sun's
+ // implementation does not check this.
+ /*
+ if (!src.isAlphaPremultiplied())
{
src1 = createCompatibleDestImage(src, src.getColorModel());
src.copyData(src1.getRaster());
src1.coerceData(true);
}
+ */
BufferedImage dst1 = dst;
- if (!src.getColorModel().equals(dst.getColorModel()))
+ if (src1.getColorModel().getColorSpace().getType() != dst.getColorModel().getColorSpace().getType())
dst1 = createCompatibleDestImage(src, src.getColorModel());
filter(src1.getRaster(), dst1.getRaster());
+ // Since we don't coerceData above, we don't need to divide it back out.
+ // This is wrong (one mauve test specifically tests converting a non-
+ // premultiplied image to a premultiplied image, and it shows that Sun
+ // simply ignores the premultipled flag, contrary to the spec), but we
+ // mimic it for compatibility.
+ /*
+ if (! dst.isAlphaPremultiplied())
+ dst1.coerceData(false);
+ */
+
+ // Convert between color models if needed
if (dst1 != dst)
- {
- // Convert between color models.
- // TODO Check that premultiplied alpha is handled correctly here.
- Graphics2D gg = dst.createGraphics();
- gg.setRenderingHints(hints);
- gg.drawImage(dst1, 0, 0, null);
- gg.dispose();
- }
-
+ new ColorConvertOp(hints).filter(dst1, dst);
+
return dst;
}
- /* (non-Javadoc)
- * @see
- * java.awt.image.BufferedImageOp#createCompatibleDestImage(java.awt.image.BufferedImage,
- * java.awt.image.ColorModel)
+ /**
+ * Creates an empty BufferedImage with the size equal to the source and the
+ * correct number of bands. The new image is created with the specified
+ * ColorModel, or if no ColorModel is supplied, an appropriate one is chosen.
+ *
+ * @param src The source image.
+ * @param dstCM A color model for the destination image (may be null).
+ * @return The new compatible destination image.
*/
public BufferedImage createCompatibleDestImage(BufferedImage src,
- ColorModel dstCM)
+ ColorModel dstCM)
{
- // FIXME: set properties to those in src
- return new BufferedImage(dstCM,
- src.getRaster().createCompatibleWritableRaster(),
- src.isPremultiplied, null);
+ if (dstCM != null)
+ return new BufferedImage(dstCM,
+ src.getRaster().createCompatibleWritableRaster(),
+ src.isAlphaPremultiplied(), null);
+
+ return new BufferedImage(src.getWidth(), src.getHeight(), src.getType());
}
/* (non-Javadoc)
@@ -168,6 +193,8 @@
}
/**
+ * Get the edge condition for this Op.
+ *
* @return The edge condition.
*/
public int getEdgeCondition()
@@ -185,9 +212,22 @@
return (Kernel) kernel.clone();
}
- /* (non-Javadoc)
- * @see java.awt.image.RasterOp#filter(java.awt.image.Raster,
- * java.awt.image.WritableRaster)
+ /**
+ * Converts the source raster using the kernel specified in the constructor.
+ * The resulting raster is stored in the destination raster if one is
+ * provided; otherwise a new WritableRaster is created and returned.
+ *
+ * If the convolved value for a sample is outside the range of [0-255], it
+ * will be clipped.
+ *
+ * The source and destination raster (if one is supplied) cannot be the same,
+ * and must also have the same dimensions.
+ *
+ * @param src The source raster.
+ * @param dest The destination raster.
+ * @throws IllegalArgumentException if the rasters identical.
+ * @throws ImagingOpException if the convolution is not possible.
+ * @return The transformed raster.
*/
public final WritableRaster filter(Raster src, WritableRaster dest)
{
@@ -228,7 +268,16 @@
v += tmp[tmp.length - i - 1] * kvals[i];
// FIXME: in the above line, I've had to reverse the order of
// the samples array to make the tests pass. I haven't worked
- // out why this is necessary.
+ // out why this is necessary.
+
+ // This clipping is pretty strange, and seems to be hard-coded
+ // at 255 (regardless of the raster's datatype or transfertype).
+ // But it's what the reference does.
+ if (v > 255)
+ v = 255;
+ if (v < 0)
+ v = 0;
+
dest.setSample(x + kernel.getXOrigin(), y + kernel.getYOrigin(),
b, v);
}
@@ -310,13 +359,14 @@
return src.getBounds();
}
- /** Return corresponding destination point for source point.
+ /**
+ * Returns the corresponding destination point for a source point. Because
+ * this is not a geometric operation, the destination and source points will
+ * be identical.
*
- * ConvolveOp will return the value of src unchanged.
* @param src The source point.
- * @param dst The destination point.
- * @see java.awt.image.RasterOp#getPoint2D(java.awt.geom.Point2D,
- * java.awt.geom.Point2D)
+ * @param dst The transformed destination point.
+ * @return The transformed destination point.
*/
public final Point2D getPoint2D(Point2D src, Point2D dst)
{
