kinow commented on code in PR #269:
URL: https://github.com/apache/commons-imaging/pull/269#discussion_r1897460841
##########
src/main/java/org/apache/commons/imaging/formats/png/PngImageParser.java:
##########
@@ -507,21 +510,61 @@ public Dimension getImageSize(final ByteSource
byteSource, final PngImagingParam
@Override
public ImageMetadata getMetadata(final ByteSource byteSource, final
PngImagingParameters params) throws ImagingException, IOException {
- final List<PngChunk> chunks = readChunks(byteSource, new ChunkType[] {
ChunkType.tEXt, ChunkType.zTXt, ChunkType.iTXt }, false);
+ final ChunkType[] chunkTypes = { ChunkType.tEXt, ChunkType.zTXt,
ChunkType.iTXt, ChunkType.eXIf };
+ final List<PngChunk> chunks = readChunks(byteSource, chunkTypes,
false);
if (chunks.isEmpty()) {
return null;
}
- final GenericImageMetadata result = new GenericImageMetadata();
+ final GenericImageMetadata textual = new GenericImageMetadata();
+ TiffImageMetadata exif = null;
for (final PngChunk chunk : chunks) {
- final AbstractPngTextChunk textChunk = (AbstractPngTextChunk)
chunk;
+ if (chunk instanceof AbstractPngTextChunk) {
+ final AbstractPngTextChunk textChunk = (AbstractPngTextChunk)
chunk;
+ textual.add(textChunk.getKeyword(), textChunk.getText());
+ } else if (chunk.getChunkType() == ChunkType.eXIf.value) {
+ if (exif != null) {
+ throw new ImagingException("Duplicate eXIf chunk");
Review Comment:
Looks good! :+1:
##########
src/main/java/org/apache/commons/imaging/formats/png/Extension.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.png;
+
+/**
+ * PNG extension types.
+ *
+ * @since 1.0-alpha6
+ */
+enum Extension {
Review Comment:
I wonder if this should be called `PngExtension`, as there are
`PngImageMetadata`, `PngImageParser`, etc. inside the `oac.imaging.formats.png`
package. Although we also have `ChunkType` :shrug: , I think that's fine.
##########
src/conf/spotbugs-exclude-filter.xml:
##########
@@ -173,6 +173,16 @@
<Method name="<init>" />
<Bug pattern="EI_EXPOSE_REP2" />
</Match>
+ <Match>
+ <Class name="org.apache.commons.imaging.formats.png.PngImageMetadata" />
+ <Method name="getExif" />
+ <Bug pattern="EI_EXPOSE_REP" />
+ </Match>
+ <Match>
+ <Class name="org.apache.commons.imaging.formats.png.PngImageMetadata" />
+ <Method name="<init>" />
+ <Bug pattern="EI_EXPOSE_REP2" />
+ </Match>
Review Comment:
These look good to me!
##########
src/main/java/org/apache/commons/imaging/formats/png/ChunkType.java:
##########
@@ -16,80 +16,217 @@
*/
package org.apache.commons.imaging.formats.png;
+import java.io.IOException;
import java.nio.charset.StandardCharsets;
import org.apache.commons.imaging.common.BinaryFunctions;
+import org.apache.commons.imaging.formats.png.chunks.PngChunk;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkGama;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkIccp;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkIdat;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkIhdr;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkItxt;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkPhys;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkPlte;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkScal;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkText;
+import org.apache.commons.imaging.formats.png.chunks.PngChunkZtxt;
/**
- * Type of a PNG chunk.
+ * Type of PNG chunk.
*
* @see <a href="https://www.w3.org/TR/png/#11Chunks";>Portable Network
Graphics Specification - Chunk specifications</a>
*/
public enum ChunkType {
- /** Image header */
- IHDR,
+ /**
+ * Image header
+ */
+ IHDR(PngChunkIhdr::new),
- /** Palette */
- PLTE,
+ /**
+ * Palette
+ */
+ PLTE(PngChunkPlte::new),
- /** Image data */
- IDAT,
+ /**
+ * Image data
+ */
+ IDAT(PngChunkIdat::new),
- /** Image trailer */
+ /**
+ * Image trailer
+ */
IEND,
- /** Transparency */
+ /**
+ * Transparency
+ */
tRNS,
- /** Primary chromaticities and white point */
+ /**
+ * Primary chromaticities and white point
+ */
cHRM,
- /** Image gamma */
- gAMA,
+ /**
+ * Image gamma
+ */
+ gAMA(PngChunkGama::new),
- /** Embedded ICC profile */
- iCCP,
+ /**
+ * Embedded ICC profile
+ */
+ iCCP(PngChunkIccp::new),
- /** Significant bits */
+ /**
+ * Significant bits
+ */
sBIT,
- /** Standard RGB color space */
+ /**
+ * Standard RGB color space
+ */
sRGB,
- /** Textual data */
- tEXt,
+ /**
+ * Textual data
+ */
+ tEXt(PngChunkText::new),
- /** Compressed textual data */
- zTXt,
+ /**
+ * Compressed textual data
+ */
+ zTXt(PngChunkZtxt::new),
- /** International textual data */
- iTXt,
+ /**
+ * International textual data
+ */
+ iTXt(PngChunkItxt::new),
- /** Background color */
+ /**
+ * Background colour
+ */
bKGD,
- /** Image histogram */
+ /**
+ * Image histogram
+ */
hIST,
- /** Physical pixel dimensions */
- pHYs,
+ /**
+ * Physical pixel dimensions
+ */
+ pHYs(PngChunkPhys::new),
- /** Physical scale */
- sCAL,
-
- /** Suggested palette */
+ /**
+ * Suggested palette
+ */
sPLT,
- /** Image last-modification time */
- tIME;
+ /**
+ * Image last-modification time
+ */
+ tIME,
+
+ /*
+ * PNGEXT
+ */
+
+ /**
+ * Image offset
+ *
+ * @since 1.0-alpha6
+ */
+ oFFs(Extension.PNGEXT),
+
+ /**
+ * Calibration of pixel values
+ *
+ * @since 1.0-alpha6
+ */
+ pCAL(Extension.PNGEXT),
+
+ /**
+ * Physical scale
+ */
+ sCAL(Extension.PNGEXT, PngChunkScal::new),
+
+ /**
+ * GIF Graphic Control Extension
+ *
+ * @since 1.0-alpha6
+ */
+ gIFg(Extension.PNGEXT),
+
+ /**
+ * GIF Application Extension
+ *
+ * @since 1.0-alpha6
+ */
+ gIFx(Extension.PNGEXT),
+
+ /**
+ * Indicator of Stereo Image
+ *
+ * @since 1.0-alpha6
+ */
+ sTER(Extension.PNGEXT),
+
+ /**
+ * Exchangeable Image File (Exif) Profile
+ *
+ * @since 1.0-alpha6
+ */
+ eXIf(Extension.PNGEXT),
+
+ ;
+
+ @FunctionalInterface
+ private interface ChunkConstructor {
+ PngChunk make(int length, int chunkType, int crc, byte[] bytes) throws
IOException;
+ }
+
+ private static final ChunkType[] types = ChunkType.values();
+
+ static ChunkType findType(int chunkType) {
+ for (ChunkType type : types) {
+ if (type.value == chunkType) {
+ return type;
+ }
+ }
+ return null;
+ }
+
+ static PngChunk makeChunk(int length, int chunkType, int crc, byte[]
bytes) throws IOException {
+ ChunkType type = findType(chunkType);
+ return type != null && type.constructor != null
+ ? type.constructor.make(length, chunkType, crc, bytes)
+ : new PngChunk(length, chunkType, crc, bytes);
+ }
Review Comment:
Good simplification here :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
