GifEncoder is final

[mufumbo]

Hi there,

not sure what's the point in making GifEncoder final,

In many implementations you would like to leverage multi threads to make the Image.fromRGB calls in parallel, rather than passing int[][] around and making Image.fromRGB from the internal API.

My use case: I download 10 images from the internet (in parallel) and create the int[][] in those threads. I could as well call Image.fromRGB from those threads, but I can't because addImage is a private method.

Furthermore, if I could extend that method, I would move everything to multiple threads, and synchronize only on ImageDataBlock.write.

If you agree, I can send a PR. The new code takes 2 seconds to generate an output that was taking 17s. I just reorganized the same code and haven't change any line.

thanks!

[JakeWharton]

It's final because it was not designed for inheritance.

I'm open to a PR that exposes the Image-taking variant of addImage.

I don't really understand how you are maintaining the desired order of images alongside parallelism. Without an intermediate object representation of all the local state prior to the calls to write, how do you avoid the racing threads and preemption from causing random order?

[mufumbo]

The bulk of scheduling can be possible by making this change to the library, in order to separate processing from write calls:
https://www.dropbox.com/s/n3llzyiz4eep69j/Screenshot%202018-04-17%2011.43.30.png?dl=0

This is the scheduling part, using the newly created method:
https://gist.github.com/mufumbo/bf0610287e350a0e0ebc5786b25c82d6

I had to do too many changes to this project, this gif was taking 17 seconds from master (and 11 seconds after adding multi threads) to be generated in 640x480 (now it renders in 1.5):
https://img.craftlog.com/m/gifs/craft-7998128-uniform_INVALIDATEME_floyd=s640=h480
https://img.craftlog.com/m/gifs/craft-7998128-uniform_INVALIDATEME_floyd=s480=h360

The MedianCut + Floyd is taking 4s now:
https://img.craftlog.com/m/gifs/craft-7998128-mediancut_INVALIDATEME_floyd=s640=h480
https://img.craftlog.com/m/gifs/craft-7998128-mediancut_INVALIDATEME_floyd=s480=h360

The bulk of optimizations:

• multi thread per frame
• deprecate the evil^2 code in Color.java: plus, scale, minus, getEuclideanDistanceTo (these were creating millions of objects inside loops of loops). I only ran on server code, but I imagine this runs very slowly on android because of GC pressure.
• Less allocations. All over the code I replaced large object deep copy and reused the same arrays instead. Example: https://www.dropbox.com/s/11rvaq21pa1l7f4/Screenshot%202018-04-17%2012.04.35.png?dl=0

I might be too far away from a PR at this point, but the entire code can be safely merged (backwards compat) if we change Dither.java to:

public interface Ditherer {
  // OLD method that creates a copy
  Image dither(Image image, Set<Color> newColors);
  // new method that dithers inline
  void ditherWithoutCopy(Image image, Set<Color> newColors);
}

That way we don't break people who depend on the copy and also give a new interface for people who don't want to waste memory. BUT, in reality nobody will ever use the deep-copy version unless they call that directly. The SDK should never make the copy in it's internals.

[swankjesse]

Submit a pull request?

[mayakwd]

@mufumbo Do you have a time to submit pull-request? Your explanation sounds as great performance boost