sksamuel/scrimage

GIFs written with StreamingGifWriter do not respect disposal method properties of original file

zalmb8 opened this issue · 3 comments

Notes

  • Using scrimage-core-4.0.26

Goal
Scale an animated GIF file to a standard size of 512x152 px

Inputs
GIF file of variable sizes and transparency

Workflow

  1. Read GIF file into application using AnimatedGifReader with a Java File object
  2. Splice original GIF into frames of ImmutableImage objects (storing delays for each frame as well)
  3. Scale each frame to 512x512 and storing the resulting ImmutableImage
  4. Using StreamingGifWriter.GifStream, write each frame with its associated delay using StreamingGifWriter.GifStream#writeFrame(ImmutableImage image, Duration delay)

Using the above, I was able to scale most GIF files no problem. When I introduced a GIF with a transparent background, there were some issues with what the GIF was doing with the previous frames while displaying the new frames. It would look like the following:
example-gif-transparency-incorrect-disposal

I eventually found that the StreamingGifWriter is defaulting everything from the node "GraphicControlExtension" (from line 89 in StreamingGifWriter) except for the delay frames which have to be manually provided when the frames are written. The underlying method GifImageWriter#getDefaultImageMetadata(), does not set disposalMethod (which defaults to 0, meaning no disposal specified). The original image specified the Restore to background color property for disposal method, which when I manually set using breakpoints and the "set value" feature in Intellij, resulted in a more expected final image.
example-gif-transparent-correct-disposal

Potential Proposed Solutions

  1. Add similar functionality for disposal method like we have for setting the frame delay when we writeFrame() using the StreamingGifWriter and have the users of the library maintain and provide that separately per frame if they care about it.
  2. (More work - see below) Maintain the metadata for each frame when spliced, so we can relay those metadata fields correctly for each frame written instead of relying on the defaults for everything except frame delay.

After some breakpoints and investigation, I noticed that when an AnimatedGif#getFrames() call was made, each frame was instantiated with the ImmutableImage.wrapAwt() that defaults metadata to ImageMetadata.empty. So at this point, any interactions with the new ImmutableImage won't remember metadata properties. We would have to rely on method calls using the original AnimatedGif object such as the AnimatedGif#getDelay() to get these properties and read them from the original frames.

I am by no means an expert in this stuff, so take my suggestions with a grain of salt, but I'd be happy to take a stab at a PR for my proposed solution number 1, but I wanted to open this first in case I'm doing something really dumb with my sample code (pasted below) that could be avoided with different usage. Thanks in advance!

-Zach

	public static void main(String[] args) throws Exception {
		String filepath = "{test.image.filepath.goes.here}";
		File file = new File(filepath);

		// read gif or apng file
		AnimatedGif originalGif = AnimatedGifReader.read(ImageSource.of(file));

		List<ImmutableImage> originalFrames = originalGif.getFrames();
		List<ImmutableImage> scaledFrames = new ArrayList<>();
		List<Duration> delayDurations = new ArrayList<>();

		// resize each frame and collect frame delays
		int i = 0;
		for(ImmutableImage originalFrame : originalFrames) {
			ImmutableImage scaledFrame = originalFrame.scaleTo(512, 512);
			scaledFrames.add(scaledFrame);
			delayDurations.add(originalGif.getDelay(i++));
		}

		// re-construct the image
		StreamingGifWriter writer = new StreamingGifWriter();
		StreamingGifWriter.GifStream newGif = writer.prepareStream("scaled.gif", BufferedImage.TYPE_INT_ARGB);

		int j = 0;
		for(ImmutableImage scaledFrame : scaledFrames) {
			newGif.writeFrame(scaledFrame, delayDurations.get(j++));
		}
		newGif.close();
	}

I like option 1. Allow the user to specify it with the frame.

First stab at a PR here: #242

I'd be happy to discuss any of the conversation points I highlighted on the PR (or others of course), just let me know!

Pr merged. Will release shortly.