Bugfix for broken transparency

[vegeta897]

I've discovered what I believe to be a bug with the transparency. Specifically, in the function that finds the nearest color in the palette. Right now transparency simply does not work at all because this function doesn't work.

This function will fail to find any color because it's checking that this.usedEntry has a true value at the index i / 3. But i is never a factor of 3 and so it's checking float index values.

The fix is changing line 302 to this:

var db = b - (this.colorTab[i++] & 0xff);

The previous 2 lines have the ++, and this line needs it as well so that i is incremented 3 times before being divided so an integer results. For some reason, the increment is at the end of the loop, so line 309 needs to be removed.

After making these changes, transparency works again.

[twolfson]

Would you be interested in opening a PR or providing a test image to reproduce/patch this?

[vegeta897]

I would, but right now I'm trying to figure out another problem with transparency not working on frames after the first.

[twolfson]

Alright, I will sit tight for now. Thanks for the bug report and quick response =)

[vegeta897]

The issue with transparency I'm running into is due to the palette generation creating duplicate entries of the same color. Because of this, the lookupRGB and findClosest functions may return different indexes (and will, more often than not, if most of the frame is transparent) and so the transparency index won't match the index that's actually used in the frame.

The palette generator is not only creating repeated entries but also including colors that aren't in the image at all. I'm guessing this is due to the compression aspect, for handling frames with >256 colors. These algorithms are too complex for me to dive into, so I'm going to resort to writing my own simple palette builder and deliberately keep my frames under 256 colors to avoid all these issues.

Sorry for not being able to help further.

[twolfson]

Can you at least provide the source images or a test case to reproduce with? =/

[vegeta897]

No source images, I'm using a canvas. Here's an example:

var Canvas = require('canvas');
var GIFEncoder = require('gifencoder');
var fs = require('fs');
var canvas = new Canvas(100, 100),
    ctx = canvas.getContext('2d');

var encoder = new GIFEncoder(100,100);
var stream = encoder.createReadStream();
stream.pipe(fs.createWriteStream(__dirname + '/test.gif'));
encoder.start();
encoder.setRepeat(0);
encoder.setDelay(50);
encoder.setTransparent(0xFF00FF);
for(var i = 0; i < 10; i++) {
    ctx.fillStyle = '#FF00FF';
    ctx.fillRect(0,0,100,100);
    ctx.fillStyle = '#000000';
    ctx.fillRect(i*10,40,10,10);
    encoder.addFrame(ctx);
}
encoder.finish();

Result with unmodified library:

Result with my initial fix, only the first frame gets working transparency:

Result with my simple <256 color palette generator, working as intended:

[twolfson]

Awesome, thanks for the test case =)

[twolfson]

I will take a look at this by the end of next weekend but maybe much sooner (e.g. today, tomorrow).

[vegeta897]

Cool! No urgency from me. I will likely stick with my palette generator unless you change that too so it doesn't generate unused or repeated colors.

[twolfson]

Taking a look at this now

[twolfson]

Ahhh, I think you have wrong repo =(

You used gifencoder as the require as well as encoder.createReadStream(). This repo is gif-encoder and doesn't have createReadStream as a method =/ I believe you are looking for:

https://github.com/eugeneware/gifencoder

[vegeta897]

Well that's embarrassing :stuck_out_tongue:

Sorry for the trouble. Maybe I'll write a test case for this lib when I have the time, I believe the same method in yours will have the same issue, as well as the palette stuff.

[twolfson]

Ah, I think you might be right. Reopening this PR as something to triage

[twolfson]

Already moved onto another task though, so going to take a look at this later =/

[twolfson]

I was able to reproduce the issue. I modularized the script a bit and modified it for our needs.

// https://github.com/twolfson/gif-encoder/issues/5
var Canvas = require('canvas');
var GIFEncoder = require('./');
var fs = require('fs');
var WIDTH = 100;
var HEIGHT = 100;
var STEP = 10;
var canvas = new Canvas(WIDTH, HEIGHT),
    ctx = canvas.getContext('2d');

var encoder = new GIFEncoder(WIDTH, HEIGHT);
encoder.pipe(fs.createWriteStream(__dirname + '/test.gif'));
encoder.setRepeat(0);
encoder.setDelay(50);
encoder.setTransparent(0xFF00FF);
encoder.writeHeader();
for(var i = 0; i < STEP; i++) {
    ctx.fillStyle = '#FF00FF';
    ctx.fillRect(0,0, WIDTH, HEIGHT);
    ctx.fillStyle = '#000000';
    ctx.fillRect(i * WIDTH / STEP, 4 * HEIGHT / STEP, WIDTH / STEP, HEIGHT / STEP);
    encoder.addFrame(ctx.getImageData(0, 0, WIDTH, HEIGHT).data);
}
encoder.finish();

Unfortunately the line 302 fix seems to break things more (e.g. the square goes hollow at some point) for me.

Since we don't have a full fix and this seems to be a deep encoding problem, I'm prob going to sit tight to see what gifencoder does =/

Thanks for the bug report =)

[vegeta897]

Sounds reasonable to me, thanks for the correspondence.

[scinos]

gifencoder has this PR pending https://github.com/eugeneware/gifencoder/pull/5, which seems to fix the problem (more or less) for me.

(disclaimer: I'm generating the GIF from a buffer of pixels, not from canvas). If you are still interested in fixing this I can open a PR with the change (or ask the original PR author to apply it to this repo instead)

[twolfson]

Yep, we are aware of that PR -- it was linked by @vegeta897 on their issue on https://github.com/eugeneware/gifencoder/issues/7

I think we expected the PR to be merged/closed by now but I guess not

I don't think I ever tried the fix on the script we wrote before. Going to try it now

[twolfson]

We don't have this.image so we can't directly using their fix. I'm trying to understand what's going on now so I can see if we can still reuse it

[scinos]

This is the hack I did:

  // get closest match to transparent color if specified
  if (this.transparent !== null) {
    this.transIndex = this.findClosest(this.transparent);

    var transR = (this.transparent & 0xFF0000) >> 16;
    var transG = (this.transparent & 0x00FF00) >> 8;
    var transB = (this.transparent & 0x0000FF);

    for (var pixelIndex = 0; pixelIndex < nPix; pixelIndex++) {
      if (
        this.pixels[pixelIndex * 3] == transR &&
        this.pixels[pixelIndex * 3 + 1] == transG &&
        this.pixels[pixelIndex * 3 + 2] == transB
      ) {
        this.indexedPixels[pixelIndex] = this.transIndex;
      }
    }
  }

Instead of looking for original pixels with full alpha, I look for pixels with the same RGB values than the transparent color.

[twolfson]

But the transparent color is still a color so it should already be in indexedPixels from:

  var k = 0;
  for (var j = 0; j < nPix; j++) {
    var index = imgq.lookupRGB(
      this.pixels[k++] & 0xff,
      this.pixels[k++] & 0xff,
      this.pixels[k++] & 0xff
    );
    this.usedEntry[index] = true;
    this.indexedPixels[j] = index;
  }

[twolfson]

The this.findClosest is attempting to find the already indexed matching color =/

[twolfson]

Alright, I don't really have time tonight to look at this further =/ I do think there are bugs in 2 places:

• findClosest is broken -- it should be using the var index = i / 3 fix
• We shouldn't be sending transparency byte when it isn't set
  • https://www.w3.org/Graphics/GIF/spec-gif89a.txt
  • "23. Graphic Control Extension"
  • We should look at this.transparent before writing this.writeByte(this.transIndex);

However, the transparency issue is eluding me =(

[twolfson]

This has been repaired by @scinos in #7 and released in 0.6.0