Fix erroneous top/left clipping in composite #2571

[SHG42]

#2571 Fixes bug where certain input values for top/left parameters
in composite can conflict with clipping logic, resulting in
inaccurate alignment in output png. Adds logic to run check
which determines if clipping needs to be applied.
Issue #2571

[coveralls]

Coverage remained the same at 100.0% when pulling 5149ada on SHG42:SHG42_composite_clipping_fix into 68ccba8 on lovell:master.

[lovell]

Thank you for the PR. If you're able to add a unit test to test/unit/composite.js with an example scenario this change fixes, that would be amazing.

[SHG42]

Hi, sure! I'm not familiar with unit tests yet, so I'll do some reading on it and get back to you when I've taken a crack at it.

[SHG42]

Hi, I'm not entirely sure how to tackle this unit test, if you have a moment could you clarify the process for me? I've reviewed the unit test file you linked, and I've done some reading on Javascript unit testing as a concept, but I'm not sure how to apply that to this specific issue, since I only discovered the bug due to a very particular set of circumstances in my project. How would I write a test that demonstrates this fix?

[lovell]

Here's a possible test case that checks colour of the top-left pixel after compositing a 2x2 blue over a 2x2 red image at an offset of 1+1.

The current version of sharp will incorrectly clip the offset and produce a top left pixel that is blue, but with the change in this PR it should be corrected to red (as only the bottom right pixel should be blue).

   it('Allow offset beyond bottom/right edge', async () => {
    const red = { r: 255, g: 0, b: 0 };
    const blue = { r: 0, g: 0, b: 255 };

    const [r, g, b] = await sharp({ create: { width: 2, height: 2, channels: 4, background: red } })
      .composite([{
        input: { create: { width: 2, height: 2, channels: 4, background: blue } },
        top: 1,
        left: 1
      }])
      .raw()
      .toBuffer();

    assert.strictEqual(red, { r, g, b });
  });

If this works, please feel free to add it as a test case to test/unit/composite.js.

[SHG42]

Hi, I keep getting a test failure on that. Just to make sure, should I be adding it at a specific line in test/unit/composite.js? I tried adding it inside describe('composite') after describe("validation"), and I also tried it inside describe("validation") after it('invalid gravity') just to be sure. I also tried both placements with both npm run test and npm run test-unit. Did I miss a step in the testing process? I'm also curious why this test uses async/await when the rest of the unit tests in the file don't?
This is the error I get:

 AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal:

{
  b: 0,
  g: 0,
  r: 255
}

[lovell]

Sorry, try deepStrictEqual rather than strictEqual - the docs for these are at https://nodejs.org/dist/latest/docs/api/assert.html

I'm slowly moving tests to using async/await where it makes sense to do so as I find it usually simplifies the flow, makes the intent clearer and offloads more error handling onto the test runner.

[SHG42]

Thanks, that worked! Gonna go ahead and add it to the test file.

[lovell]

Brilliant, thank you for fixing this, I'll get this merged for v0.28.0.

[lovell]

Landed via commit 34a2e14