Making GIFs with Java

Question

I wrote a Java class to make a GIF animation from a list of images. The whole project can be found here.

I am pretty new with GitHub, so I would be very glad if you can give critiques regarding my project structure.

package shine.htetaung.giffer;

import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;
import java.util.Iterator;

import javax.imageio.IIOException;
import javax.imageio.IIOImage;
import javax.imageio.ImageIO;
import javax.imageio.ImageTypeSpecifier;
import javax.imageio.ImageWriter;
import javax.imageio.metadata.IIOInvalidTreeException;
import javax.imageio.metadata.IIOMetadata;
import javax.imageio.metadata.IIOMetadataNode;
import javax.imageio.stream.ImageOutputStream;

/*
 * Giffer is a simple java class to make my life easier in creating gif images.
 * 
 * Usage :
 * There are two methods for creating gif images
 * To generate from files, just pass the array of filename Strings to this method
 * Giffer.generateFromFiles(String[] filenames, String output, int delay, boolean loop)
 * 
 * Or as an alternative you can use this method which accepts an array of BufferedImage
 * Giffer.generateFromBI(BufferedImage[] images, String output, int delay, boolean loop)
 * 
 * output is the name of the output file
 * delay is time between frames, accepts hundredth of a time. Yeah it's weird, blame Oracle
 * loop is the boolean for whether you want to make the image loopable.
 */

public abstract class Giffer {

    // Generate gif from an array of filenames
    // Make the gif loopable if loop is true
    // Set the delay for each frame according to the delay (ms)
    // Use the name given in String output for output file
    public static void generateFromFiles(String[] filenames, String output, int delay, boolean loop)
        throws IIOException, IOException
    {
        int length = filenames.length;
        BufferedImage[] img_list = new BufferedImage[length];

        for (int i = 0; i < length; i++)
        {
            BufferedImage img = ImageIO.read(new File(filenames[i]));
            img_list[i] = img;
        }

        generateFromBI(img_list, output, delay, loop);
    }

    // Generate gif from BufferedImage array
    // Make the gif loopable if loop is true
    // Set the delay for each frame according to the delay, 100 = 1s
    // Use the name given in String output for output file
    public static void generateFromBI(BufferedImage[] images, String output, int delay, boolean loop)
            throws IIOException, IOException
    {
        ImageWriter gifWriter = getWriter();
        ImageOutputStream ios = getImageOutputStream(output);
        IIOMetadata metadata = getMetadata(gifWriter, delay, loop);

        gifWriter.setOutput(ios);
        gifWriter.prepareWriteSequence(null);
        for (BufferedImage img: images)
        {
            IIOImage temp = new IIOImage(img, null, metadata);
            gifWriter.writeToSequence(temp, null);
        }
        gifWriter.endWriteSequence();
    }

    // Retrieve gif writer
    private static ImageWriter getWriter() throws IIOException
    {
        Iterator<ImageWriter> itr = ImageIO.getImageWritersByFormatName("gif");
        if(itr.hasNext())
            return itr.next();

        throw new IIOException("GIF writer doesn't exist on this JVM!");
    }

    // Retrieve output stream from the given file name
    private static ImageOutputStream getImageOutputStream(String output) throws IOException
    {
        File outfile = new File(output);
        return ImageIO.createImageOutputStream(outfile);
    }

    // Prepare metadata from the user input, add the delays and make it loopable
    // based on the method parameters
    private static IIOMetadata getMetadata(ImageWriter writer, int delay, boolean loop)
        throws IIOInvalidTreeException
    {
        // Get the whole metadata tree node, the name is javax_imageio_gif_image_1.0
        // Not sure why I need the ImageTypeSpecifier, but it doesn't work without it
        ImageTypeSpecifier img_type = ImageTypeSpecifier.createFromBufferedImageType(BufferedImage.TYPE_INT_ARGB);
        IIOMetadata metadata = writer.getDefaultImageMetadata(img_type, null);
        String native_format = metadata.getNativeMetadataFormatName();
        IIOMetadataNode node_tree = (IIOMetadataNode)metadata.getAsTree(native_format);

        // Set the delay time you can see the format specification on this page
        // https://docs.oracle.com/javase/7/docs/api/javax/imageio/metadata/doc-files/gif_metadata.html
        IIOMetadataNode graphics_node = getNode("GraphicControlExtension", node_tree);
        graphics_node.setAttribute("delayTime", String.valueOf(delay));
        graphics_node.setAttribute("disposalMethod", "none");
        graphics_node.setAttribute("userInputFlag", "FALSE");

        if(loop)
            makeLoopy(node_tree);

        metadata.setFromTree(native_format, node_tree);

        return metadata;
    }

    // Add an extra Application Extension node if the user wants it to be loopable
    // I am not sure about this part, got the code from StackOverflow
    // TODO: Study about this
    private static void makeLoopy(IIOMetadataNode root)
    {
        IIOMetadataNode app_extensions = getNode("ApplicationExtensions", root);
        IIOMetadataNode app_node = getNode("ApplicationExtension", app_extensions);

        app_node.setAttribute("applicationID", "NETSCAPE");
        app_node.setAttribute("authenticationCode", "2.0");
        app_node.setUserObject(new byte[]{ 0x1, (byte) (0 & 0xFF), (byte) ((0 >> 8) & 0xFF)});

        app_extensions.appendChild(app_node);
        root.appendChild(app_extensions);
    }

    // Retrieve the node with the name from the parent root node
    // Append the node if the node with the given name doesn't exist
    private static IIOMetadataNode getNode(String node_name, IIOMetadataNode root)
    {
        IIOMetadataNode node = null;

        for (int i = 0; i < root.getLength(); i++)
        {
            if(root.item(i).getNodeName().compareToIgnoreCase(node_name) == 0)
            {
                node = (IIOMetadataNode) root.item(i);
                return node;
            }
        }

        // Append the node with the given name if it doesn't exist
        node = new IIOMetadataNode(node_name);
        root.appendChild(node);

        return node;
    }

    public static void main(String[] args)
    {
        String[] img_strings = {"sample-images/cool.png", "sample-images/cry.png", "sample-images/love.png", "sample-images/oh.png"};

        try
        {
            Giffer.generateFromFiles(img_strings, "sample-images/output.gif", 40, true);
        } catch (Exception ex) 
        {
            ex.printStackTrace();
        }
    }
}

Answer 1

I agree with Peter Rader that static factories are not a good pattern. For this particular task I would be inclined to use a builder pattern. In particular, I would quite like to have a BufferedImage... images varargs parameter somewhere, and IMO the cleanest way of doing that would be for it to be the sole parameter of a constructor.

---

generateFromBI opens an output stream, but doesn't seem to close it. The "best practices" way of doing this nowadays would be a try-with-resources statement. I haven't used Java 8, so I can't do better than to point you at the docs.

---

    // Not sure why I need the ImageTypeSpecifier, but it doesn't work without it
    ImageTypeSpecifier img_type = ImageTypeSpecifier.createFromBufferedImageType(BufferedImage.TYPE_INT_ARGB);

Full marks for honest commenting, but you may not have realised that you're making a dangerous assumption. What if the image isn't of TYPE_INT_ARGB? The more robust way of doing this would be to create a fresh IIOMetadata for each image, using new ImageTypeSpecifier(img) to create an appropriate ImageTypeSpecifier for each one.

---

The disposalMethod is hard-coded to "none". It would be nice to make this configurable, preferably via an enum rather than raw strings, in particular because using "doNotDispose" opens up the possibility of optimising the images to use transparency on areas which don't change.

(And on the subject of exposing nice types rather than cryptic raw data, Java 8 provides Duration, which is nicer than raw centiseconds).

---

       app_node.setUserObject(new byte[]{ 0x1, (byte) (0 & 0xFF), (byte) ((0 >> 8) & 0xFF)});

is a bit odd. It seems to be saying "The structure here is very important and you might want to change it, but I won't document it for you". I would either use magic new byte[] { 1, 0, 0 } or pull out the parameters as new byte[]{ foo, bar, baz } with more descriptive names.

---

Finally, some of the method names seem slightly misleading to me. get connotes "this object already exists and is stored somewhere", but several of these getXYZ methods are actually creating (or getting, with a creation fallback). I would favour renaming with a create or a findOrCreate prefix as appropriate for each case.

Answer 2

I expected to find a oldschool factory pattern, not a pseudo static-method-factory-pattern ... static methods always makes me a little bit sad because you can not mock static-programming-units in tests very well.

The method getWriter is essential! It makes no sense to load the class if getWriter fails anyway. Also classes shall report problems as early as possible. See this:

public abstract class Giffer {
   private static final ImageWriter defaultWriter = 
       ImageIO.getImageWritersByFormatName("gif").next();
   ...
}

The class would fail as early as possible.

Instead of set the Files via String there is a better matching class for this: File.

Try to follow the packagename conventions.