Hello Andrew,
Andrew Brygin wrote:
> fix looks fine for me in general, just one minor note:
Thanks for looking at this. Let me know when you think it ready for
inclusion.
> There is getBooleanAttribute() method in the GIFMetadata class, which
> probably should be modified as a part of this fix. This method uses
> equalsIgnoreCase() to compare attribute values and refers this bug
> as a reason for this "too tolerant" approach. It also seems to be
> inconsistent
> with suggested implementation of similar method in the PNGMetadata class.
I had mentioned that issue with GIFMetadata in my mail, yes.
I now made GIFMetadata behave like PNGMetadata, i.e. only allow strictly
upper or strictly lower case, no mixed forms.
> Could you also provide a regression test for this change?
I wrote a test case. For the formats PNG, JPEG and GIF it merges a
native tree containing uppercase versions of every possible boolean
value. It then verifies that in the tree generated by asTree the values
are still uppercase. The verification is repeated again after the images
got written to a stream and were read back. Standard trees are verified
as well, though not merged with.
The text class divides roughly into two parts. One listing the test
cases, consisting of MIME type, Metadata XML and XPath expressions of
the values to be checked, the other performs the specified tests. The
framework seems pretty flexible and might well be used for other
metadata-related tests as well. Do you agree with the imageio/metadata
directory, as it affects multiple plugins and duplicating the code seems
a bad idea?
Greetings,
Martin
diff --git
a/src/share/classes/com/sun/imageio/plugins/gif/GIFImageMetadata.java
b/src/share/classes/com/sun/imageio/plugins/gif/GIFImageMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/gif/GIFImageMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/gif/GIFImageMetadata.java
@@ -153,7 +153,7 @@
node.setAttribute("imageWidth", Integer.toString(imageWidth));
node.setAttribute("imageHeight", Integer.toString(imageHeight));
node.setAttribute("interlaceFlag",
- interlaceFlag ? "true" : "false");
+ interlaceFlag ? "TRUE" : "FALSE");
root.appendChild(node);
// Local color table
@@ -185,9 +185,9 @@
node.setAttribute("disposalMethod",
disposalMethodNames[disposalMethod]);
node.setAttribute("userInputFlag",
- userInputFlag ? "true" : "false");
+ userInputFlag ? "TRUE" : "FALSE");
node.setAttribute("transparentColorFlag",
- transparentColorFlag ? "true" : "false");
+ transparentColorFlag ? "TRUE" : "FALSE");
node.setAttribute("delayTime",
Integer.toString(delayTime));
node.setAttribute("transparentColorIndex",
diff --git a/src/share/classes/com/sun/imageio/plugins/gif/GIFMetadata.java
b/src/share/classes/com/sun/imageio/plugins/gif/GIFMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/gif/GIFMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/gif/GIFMetadata.java
@@ -158,13 +158,10 @@
}
}
String value = attr.getNodeValue();
- // XXX Should be able to use equals() here instead of
- // equalsIgnoreCase() but some boolean attributes are incorrectly
- // set to "true" or "false" by the J2SE core metadata classes
- // getAsTree() method (which are duplicated above). See bug 5082756.
- if (value.equalsIgnoreCase("TRUE")) {
+ // Allow lower case booleans for backward compatibility, #5082756
+ if (value.equals("TRUE") || value.equals("true")) {
return true;
- } else if (value.equalsIgnoreCase("FALSE")) {
+ } else if (value.equals("FALSE") || value.equals("false")) {
return false;
} else {
fatal(node, "Attribute " + name + " must be 'TRUE' or 'FALSE'!");
diff --git
a/src/share/classes/com/sun/imageio/plugins/gif/GIFStreamMetadata.java
b/src/share/classes/com/sun/imageio/plugins/gif/GIFStreamMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/gif/GIFStreamMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/gif/GIFStreamMetadata.java
@@ -202,7 +202,7 @@
compression_node.appendChild(node);
node = new IIOMetadataNode("Lossless");
- node.setAttribute("value", "true");
+ node.setAttribute("value", "TRUE");
compression_node.appendChild(node);
// NumProgressiveScans not in stream
diff --git a/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGMetadata.java
b/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/jpeg/JPEGMetadata.java
@@ -955,7 +955,7 @@
// Lossless - false
IIOMetadataNode lossless = new IIOMetadataNode("Lossless");
- lossless.setAttribute("value", "false");
+ lossless.setAttribute("value", "FALSE");
compression.appendChild(lossless);
// NumProgressiveScans - count sos segments
diff --git a/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
b/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
--- a/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
+++ b/src/share/classes/com/sun/imageio/plugins/png/PNGMetadata.java
@@ -832,7 +832,7 @@
}
node = new IIOMetadataNode("BlackIsZero");
- node.setAttribute("value", "true");
+ node.setAttribute("value", "TRUE");
chroma_node.appendChild(node);
if (PLTE_present) {
@@ -894,7 +894,7 @@
compression_node.appendChild(node);
node = new IIOMetadataNode("Lossless");
- node.setAttribute("value", "true");
+ node.setAttribute("value", "TRUE");
compression_node.appendChild(node);
node = new IIOMetadataNode("NumProgressiveScans");
@@ -1162,12 +1162,13 @@
}
}
String value = attr.getNodeValue();
- if (value.equals("true")) {
+ // Allow lower case booleans for backward compatibility, #5082756
+ if (value.equals("TRUE") || value.equals("true")) {
return true;
- } else if (value.equals("false")) {
+ } else if (value.equals("FALSE") || value.equals("false")) {
return false;
} else {
- fatal(node, "Attribute " + name + " must be 'true' or 'false'!");
+ fatal(node, "Attribute " + name + " must be 'TRUE' or 'FALSE'!");
return false;
}
}
diff --git a/test/javax/imageio/metadata/BooleanAttributes.java
b/test/javax/imageio/metadata/BooleanAttributes.java
new file mode 100644
--- /dev/null
+++ b/test/javax/imageio/metadata/BooleanAttributes.java
@@ -0,0 +1,173 @@
+/*
+ * @test
+ * @bug 5082756
+ * @summary ensure that boolean attributes follow ( "TRUE" | "FALSE" )
+ * including correct (i.e. upper) case
+ *
+ * @run main BooleanAttributes
+ */
+
+import java.awt.image.BufferedImage;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.StringReader;
+import java.util.Arrays;
+import java.util.List;
+import javax.imageio.IIOImage;
+import javax.imageio.ImageIO;
+import javax.imageio.ImageReader;
+import javax.imageio.ImageTypeSpecifier;
+import javax.imageio.ImageWriteParam;
+import javax.imageio.ImageWriter;
+import javax.imageio.metadata.IIOMetadata;
+import javax.imageio.stream.ImageInputStream;
+import javax.imageio.stream.ImageOutputStream;
+import javax.imageio.stream.MemoryCacheImageInputStream;
+import javax.imageio.stream.MemoryCacheImageOutputStream;
+import javax.xml.transform.Result;
+import javax.xml.transform.Source;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.dom.DOMResult;
+import javax.xml.transform.stream.StreamSource;
+import javax.xml.xpath.XPath;
+import javax.xml.xpath.XPathConstants;
+import javax.xml.xpath.XPathFactory;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+
+class BooleanAttributes {
+
+ private static TransformerFactory transformerFactory =
+ TransformerFactory.newInstance();
+
+ private static XPath xpathEngine = XPathFactory.newInstance().newXPath();
+
+ public static void main(String[] args) throws Exception {
+ test("image/png", false, "<javax_imageio_1.0 />",
+ "Chroma/BlackIsZero/@value",
+ "Compression/Lossless/@value");
+ test("image/png", false,
+ "<javax_imageio_png_1.0>" +
+ "<iTXt><iTXtEntry keyword='Comment' compressionFlag='TRUE' " +
+ "compressionMethod='0' languageTag='en' " +
+ "translatedKeyword='comment' text='foo'/></iTXt>" +
+ "</javax_imageio_png_1.0>",
+ "iTXt/iTXtEntry/@compressionFlag");
+ test("image/png", false,
+ "<javax_imageio_png_1.0>" +
+ "<iTXt><iTXtEntry keyword='Comment' compressionFlag='FALSE' " +
+ "compressionMethod='0' languageTag='en' " +
+ "translatedKeyword='comment' text='foo'/></iTXt>" +
+ "</javax_imageio_png_1.0>",
+ "iTXt/iTXtEntry/@compressionFlag");
+
+ test("image/gif", false, "<javax_imageio_1.0 />",
+ "Chroma/BlackIsZero/@value",
+ "Compression/Lossless/@value");
+ test("image/gif", false,
+ "<javax_imageio_gif_image_1.0>" +
+ "<ImageDescriptor imageLeftPosition='0' imageTopPosition='0' " +
+ "imageWidth='16' imageHeight='16' interlaceFlag='TRUE' />" +
+ "<LocalColorTable sizeOfLocalColorTable='2' " +
+ "backgroundColorIndex='1' sortFlag='TRUE'>" +
+ "<ColorTableEntry index='0' red='0' green='0' blue='0' />" +
+ "<ColorTableEntry index='1' red='255' green='255' blue='255' />" +
+ "</LocalColorTable>" +
+ "<GraphicControlExtension disposalMethod='doNotDispose' " +
+ "userInputFlag='FALSE' transparentColorFlag='TRUE' " +
+ "delayTime='100' transparentColorIndex='1' />" +
+ "</javax_imageio_gif_image_1.0>",
+ "ImageDescriptor/@interlaceFlag",
+ "LocalColorTable/@sortFlag",
+ "GraphicControlExtension/@userInputFlag",
+ "GraphicControlExtension/@transparentColorFlag");
+ test("image/gif", true,
+ "<javax_imageio_gif_stream_1.0>" +
+ "<GlobalColorTable sizeOfGlobalColorTable='2' " +
+ "backgroundColorIndex='1' sortFlag='TRUE'>" +
+ "<ColorTableEntry index='0' red='0' green='0' blue='0' />" +
+ "<ColorTableEntry index='1' red='255' green='255' blue='255' />" +
+ "</GlobalColorTable>" +
+ "</javax_imageio_gif_stream_1.0>",
+ "GlobalColorTable/@sortFlag");
+
+ test("image/jpeg", false, "<javax_imageio_1.0 />",
+ "Compression/Lossless/@value");
+ }
+
+ private static void transform(Source src, Result dst)
+ throws Exception
+ {
+ transformerFactory.newTransformer().transform(src, dst);
+ }
+
+ private static void verify(Node meta, String[] xpaths, boolean required)
+ throws Exception
+ {
+ for (String xpath: xpaths) {
+ NodeList list = (NodeList)
+ xpathEngine.evaluate(xpath, meta, XPathConstants.NODESET);
+ if (list.getLength() == 0 && required)
+ throw new AssertionError("Missing value: " + xpath);
+ for (int i = 0; i < list.getLength(); ++i) {
+ String value = list.item(i).getNodeValue();
+ if (!(value.equals("TRUE") || value.equals("FALSE")))
+ throw new AssertionError(xpath + " has value " + value);
+ }
+ }
+ }
+
+ public static void test(String mimeType, boolean useStreamMeta,
+ String metaXml, String... boolXpaths)
+ throws Exception
+ {
+ BufferedImage img =
+ new BufferedImage(16, 16, BufferedImage.TYPE_INT_RGB);
+ ImageWriter iw = ImageIO.getImageWritersByMIMEType(mimeType).next();
+ ByteArrayOutputStream os = new ByteArrayOutputStream();
+ ImageOutputStream ios = new MemoryCacheImageOutputStream(os);
+ iw.setOutput(ios);
+ ImageWriteParam param = null;
+ IIOMetadata streamMeta = iw.getDefaultStreamMetadata(param);
+ IIOMetadata imageMeta =
+ iw.getDefaultImageMetadata(new ImageTypeSpecifier(img), param);
+ IIOMetadata meta = useStreamMeta ? streamMeta : imageMeta;
+ Source src = new StreamSource(new StringReader(metaXml));
+ DOMResult dst = new DOMResult();
+ transform(src, dst);
+ Document doc = (Document)dst.getNode();
+ Element node = doc.getDocumentElement();
+ String metaFormat = node.getNodeName();
+
+ // Verify that the default metadata gets formatted correctly.
+ verify(meta.getAsTree(metaFormat), boolXpaths, false);
+
+ meta.mergeTree(metaFormat, node);
+
+ // Verify that the merged metadata gets formatte correctly.
+ verify(meta.getAsTree(metaFormat), boolXpaths, true);
+
+ iw.write(streamMeta, new IIOImage(img, null, imageMeta), param);
+ iw.dispose();
+ ios.close();
+ ImageReader ir = ImageIO.getImageReader(iw);
+ byte[] bytes = os.toByteArray();
+ if (bytes.length == 0)
+ throw new AssertionError("Zero length image file");
+ ByteArrayInputStream is = new ByteArrayInputStream(bytes);
+ ImageInputStream iis = new MemoryCacheImageInputStream(is);
+ ir.setInput(iis);
+ if (useStreamMeta) meta = ir.getStreamMetadata();
+ else meta = ir.getImageMetadata(0);
+
+ // Verify again after writing and re-reading the image
+ verify(meta.getAsTree(metaFormat), boolXpaths, true);
+ }
+
+ public static void xtest(Object... eatAnyArguments) {
+ System.err.println("Disabled test! Change xtest back into test!");
+ }
+
+}
