twolfson / gif-encoder

Streaming GIF encoder
The Unlicense
86 stars 10 forks source link

Add support for user-supplied palette + indexed pixel input #15

Closed nurpax closed 6 years ago

nurpax commented 6 years ago

Quantizing (palette computation + remapping RGB to indexed) is a slow process. If the gif encoder inputs already happen to be indexed (which it often is when say encoding pixel art into gif anims), this whole process can be skipped.

This patch allows specifying a palette once on GIFEncoder creation and makes it possible to pass indexed inputs to GIFEncoder.addFrame.

This is somewhat of a prototype. Can clean this up if it looks like this has a chance to be merged.

twolfson commented 6 years ago

Sorry for the slow reply, was on vacation. Will review by the end of next week. Seems fine at a glance

twolfson commented 6 years ago

Looks good to me. What were the ideas you had for clean-up?

I think before I land I'll readjust everything so it's all var's still for consistency as well as add documentation but it sounds like you had other ideas

nurpax commented 6 years ago

Now indexed vs rgb pixel data is heuristically detected based on pixels array length. A more orthogonal implementation would support all these 3 options:

  1. RGB pixels, compute palette, quantize
  2. RGB pixels + input palette, quantize to given palette (not supported in my patch)
  3. indexed pixels + input palette, basically just pass through everything (this is what I added)

Option 2 would be nice to have as the API would be a bit more orthogonal.

But #3 is what I was after I needed in https://github.com/nurpax/petmate so I just implemented that. Probably makes sense to just merge after any clean up you might have in mind.

nurpax commented 6 years ago

Maybe gifencoder options could have a pixelFormat field which could be either RGBA, RGB (can’t recall which you have now) or indexed. This would be explicit and similar to what other imaging apis might have.

twolfson commented 6 years ago

I'd prefer to stick to a consistent RGBA input format to avoid changing the API too much. I'm more considering this repo to be in stable API mode than anything else due to my lack of time

I think I'm realizing that the custom palette lookup might be a pain to implement so I'll take some more time to think about it. I'll reply again by the end of next week

twolfson commented 6 years ago

Sorry for the delay. I've sided with keeping the PR mostly as is but making the API flexible for option (2) in the future

This should look like:

var gif = new GIF({palette: ...});
gif.addFrame({
  indexedPixels: true
}, [0, 1, ...]);

No need to update anything on your end. I'm going to implement these changes on top of the PR to minimize future back/forths

twolfson commented 6 years ago

Okay, done with the updates and released it in 0.7.0

As a heads up, the calculation for palSize was incorrect -- it's always 7 due to using an 8-bit color table. GIFEncoder.js was correcting this inside of analyzePixels:

https://github.com/twolfson/gif-encoder/blob/0.6.1/lib/GIFEncoder.js#L278

I also adjusted the palette code to properly represent what the software is currently doing which is writing a new palette every frame

var gif = new GIF(10, 10);
var palette = [0x00, 0x00, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF];
gif.addFrame([
  0, 1, ...
], {
  palette: palette,
  indexedPixels: true
});

I'm happy to accept a PR which makes it not do that but I felt it was lying through its teeth

Thanks for the PR and quick back/forths!