kinow commented on code in PR #254: URL: https://github.com/apache/commons-imaging/pull/254#discussion_r1056979055
########## src/main/java/org/apache/commons/imaging/formats/webp/chunks/WebPChunkVP8.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.webp.chunks; + +import org.apache.commons.imaging.ImageReadException; + +import java.io.IOException; +import java.io.PrintWriter; + +public final class WebPChunkVP8 extends WebPChunk { + private final int versionNumber; + private final int width; + private final int height; + private final int horizontalScale; + private final int verticalScale; + + public WebPChunkVP8(int type, int size, byte[] bytes) throws ImageReadException { + super(type, size, bytes); + + if (size < 10) { + throw new ImageReadException("Invalid VP8 chunk"); + } + + int b0 = bytes[0] & 0xFF; + // int b1 = bytes[1] & 0xFF; + // int b2 = bytes[2] & 0xFF; Review Comment: I like that these two are commented out, meaning that it was not a mistake to skip these. But we still need a comment, so other devs are aware of why this was done, please. ########## src/main/java/org/apache/commons/imaging/formats/webp/chunks/WebPChunk.java: ########## @@ -0,0 +1,80 @@ +/* + * 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.webp.chunks; + +import org.apache.commons.imaging.ImageReadException; +import org.apache.commons.imaging.common.BinaryFileParser; + +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; + +public abstract class WebPChunk extends BinaryFileParser { + public static final int TYPE_VP8 = 0x20385056; + public static final int TYPE_VP8L = 0x4C385056; + public static final int TYPE_VP8X = 0x58385056; + public static final int TYPE_ANIM = 0x4D494E41; + public static final int TYPE_ANMF = 0x464D4E41; + public static final int TYPE_ICCP = 0x50434349; + public static final int TYPE_EXIF = 0x46495845; + public static final int TYPE_XMP = 0x20504D58; + + private final int type; + private final int size; + protected final byte[] bytes; + + WebPChunk(int type, int size, byte[] bytes) throws ImageReadException { + super(ByteOrder.LITTLE_ENDIAN); + + if (size != bytes.length) { + throw new AssertionError("Chunk size must match bytes length"); + } + + this.type = type; + this.size = size; + this.bytes = bytes; + } + + public int getType() { + return type; + } + + public String getTypeDescription() { + return new String(new byte[]{ + (byte) (type & 0xff), + (byte) ((type >> 8) & 0xff), + (byte) ((type >> 16) & 0xff), + (byte) ((type >> 24) & 0xff)}, StandardCharsets.UTF_8); + } + + public int getPayloadSize() { + return size; + } + + public int getChunkSize() { + return size + 8 + ((size % 2) == 0 ? 0 : 1); Review Comment: This is a good example of something that's not too complicated, but would be nicer to have a short explanation why we are doing it. If it's in the spec, you can just say so :+1: ########## src/main/java/org/apache/commons/imaging/formats/webp/chunks/WebPChunk.java: ########## @@ -0,0 +1,80 @@ +/* + * 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.webp.chunks; + +import org.apache.commons.imaging.ImageReadException; +import org.apache.commons.imaging.common.BinaryFileParser; + +import java.io.IOException; +import java.io.PrintWriter; +import java.nio.ByteOrder; +import java.nio.charset.StandardCharsets; + +public abstract class WebPChunk extends BinaryFileParser { + public static final int TYPE_VP8 = 0x20385056; + public static final int TYPE_VP8L = 0x4C385056; + public static final int TYPE_VP8X = 0x58385056; + public static final int TYPE_ANIM = 0x4D494E41; + public static final int TYPE_ANMF = 0x464D4E41; + public static final int TYPE_ICCP = 0x50434349; + public static final int TYPE_EXIF = 0x46495845; + public static final int TYPE_XMP = 0x20504D58; + + private final int type; + private final int size; + protected final byte[] bytes; + + WebPChunk(int type, int size, byte[] bytes) throws ImageReadException { + super(ByteOrder.LITTLE_ENDIAN); + + if (size != bytes.length) { + throw new AssertionError("Chunk size must match bytes length"); Review Comment: Instead of `AssertionError` I think it would be best to throw the `ImageReadException` ########## src/main/java/org/apache/commons/imaging/formats/webp/WebPImagingParameters.java: ########## @@ -0,0 +1,22 @@ +/* + * 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.webp; + +import org.apache.commons.imaging.common.XmpImagingParameters; + +public class WebPImagingParameters extends XmpImagingParameters { Review Comment: Even small public methods and classes need a javadoc. It normally must include - brief comment - @since tag - iff it's useful, tell users whether it's thread-safe or not - if it contains complicated code, you can have a longer comment, include code examples, links to spec, etc. ########## src/main/java/org/apache/commons/imaging/formats/webp/chunks/WebPChunkXYZW.java: ########## @@ -0,0 +1,25 @@ +/* + * 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.webp.chunks; + +import org.apache.commons.imaging.ImageReadException; + +public final class WebPChunkXYZW extends WebPChunk { + public WebPChunkXYZW(int type, int size, byte[] bytes) throws ImageReadException { Review Comment: We need javadocs for public methods at least. If there are internal/private methods, that are not easy to understand (e.g. complex logic, lots of byte handling) it's also recommended to have some javadocs & code comments. ########## src/main/java/org/apache/commons/imaging/formats/webp/WebPImagingParameters.java: ########## @@ -0,0 +1,22 @@ +/* + * 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.webp; + +import org.apache.commons.imaging.common.XmpImagingParameters; + +public class WebPImagingParameters extends XmpImagingParameters { Review Comment: e.g. https://github.com/apache/commons-imaging/blob/cb0b52e5b82321cac1d6b4c97db7d58375322733/src/main/java/org/apache/commons/imaging/formats/png/PngImagingParameters.java#L23-L26 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
