Parameterize ImageMagick library selection.

[x3au]

Load ImageMagick library based on the parameter value.

[twolfson]

Ah, I'm realizing that there is a very specific way I want #32 done. Effectively, if I tell you how to do it, then I will be writing the code so I guess I should just write the damn code.

Sorry for wasting your time on this x_x

[twolfson]

For reference, problems in this current iteration:

• All instances of ImageDiff are using a common global gm (should be per-instance)
• We have a require inlined in a method, not done at load time which causes sync loading =(
• We are still using compare instead of gm in a location

[x3au]

Thanks for the feedback. To be honest, I am new to javascript/node, so would be happy to use this opportunity to learn. If it's convenient for you to make changes, that's great as well.

[twolfson]

Alright, I can do that

[twolfson]

We are going to want an instance bound gm to prevent sharing a common gm instance across different image diffs (e.g. someone might want to run both GraphicsMagick and ImageMagick diffs in the same script)

[twolfson]

To do that, we should start accepting options in function ImageDiff and creating an instance of gm there.

// Load in `gm` dependency via `require` at the top
var gm = require('gm');

// Inside of the constructor, we adjust `gm` as needed:
function ImageDiff(options) {
  var useImageMagick = options.imageMagick === undefined || options.imageMagick;
  this.gm = useImageMagick ? gm.subclass({imageMagick: true}) : gm;
}

[twolfson]

Unfortunately, this does pollute the prototype now that we can be passing in gm directly without imageMagick subclassing. As a result, we should now make this a vanilla function:

function fillFromTo(gm, params) {
  gm.borderColor('transparent');
  // ...
  return gm
}

// When used
fillFromTo(this.gm(actualPath), {
  fromWidth: actualSize.width,
  fromHeight: actualSize.height,
  toWidth: maxWidth,
  toHeight: maxHeight
})

[twolfson]

Left some comments to resolve, additionally we need to take care of the compare call. We should be able to do something like:

this.diffCmd = useImageMagick ? 'compare' : 'gm';
this.baseDiffArgs = useImageMagick ? [] : ['compare'];

// Then in the compare call
var diffArgs = this.baseDiffArgs.concat([
  '-verbose',
  // ...
])

[x3au]

Cool, will give a try, thanks!

[x3au]

I made all the changes except the "compare" call. When I test locally with imageMagick set to false, I stumbled on the same issue that I had faced earlier which triggered this change. The call below creates a one pixel file for expectedTmpPath and actualTmpPath:

fillFromTo(this.gm(actualPath), {
                fromWidth: actualSize.width,
                fromHeight: actualSize.height,
                toWidth: maxWidth,
                toHeight: maxHeight
              }).write(actualTmpPath, cb);

Any ideas what could cause this issue? Before these changes, the problem occurred only for ImageMagick library, now I see it happening for GraphicsMagick as well.

Thanks. (pushing the code changes for you to get an idea)

[twolfson]

Can you upload the images you are comparing?

[x3au]

[x3au]

Uploaded the images. What I had discovered was the crop function creates one pixel file if parameters passed are incorrect.

[twolfson]

So does that mean everything is working as expected?

[x3au]

I don't think so. Image comparison always succeeds no matter what images are because the comparison is taking place between expectedTmpPath and actualTmpPath, both of which are 1 pixel file. This happens in following scenarios:

image-diff 1.3.0 on master - issue occurs
changeset 579fb8c with imageMagick set to false - does not occur
changeset 84a6ef4 with imageMagick set to false or true - issue occurs

Not sure what the root cause is.

[twolfson]

Ah, sorry. I meant on the unaltered master branch of this project (i.e. tag 1.3.0/commit d9dc82030b5d77d829c484151b7716290177175e)

[x3au]

1.3.0/commit d9dc820 has the issue, which is why I started with this approach.

[twolfson]

Ah, I have created a gist to reproduce the issue. It looks like a problem with the library, not your OS nor ImageMagick nor GraphicsMagick.

https://gist.github.com/twolfson/710485d2941d0e19b5d5

[twolfson]

After a bit of digging, it looks like we were using this.crop instead of this.extent x_x After making that change, everything seems to work as expected

[x3au]

Great, hope you can get this into master soon. Thanks again!

[twolfson]

I got something working locally but it broke again with the example images you provided. Unfortunately, I have to take off for the night. I will take a look at getting it resolved by the end of the weekend.

[x3au]

Sounds good, thanks!

[twolfson]

A replacement PR has been opened in #33, can you please close this PR?

[twolfson]

Sorry, I meant #34