Automatic cropping doesn't always respect scale=down

[lilith]

Bug in ImageBuilder.cs LayoutImage method:

Automatic cropping can bypass the scale=down checks in certain scenarios, such as when the requested and original aspect ratio differ significantly, and the uncropped (but resized) image would normally not cause upscaling.

Solving this without breaking things will require unit tests.

[lilith]

When &scale=down, upscaling should never occur. Instead, the image should only be cropped sufficiently to meet the size restrictions, not to meet the aspect ratio.

Example: Source image of 300x400

• Querystring ?width=400&height=300&mode=crop&scale=down
• Current result: 400x300 cropped, upscaled image
• Correct result: 300x300 cropped, unresized image.

---

• Querystring ?width=400&height=300&mode=crop&scale=canvas
• Current result: 400x300 cropped, upscaled image
• Correct result: 400x300 partially cropped, unresized image, padded to fit remainder.

---

• Querystring ?width=400&height=300&mode=crop&scale=both
• Current result: 400x300 cropped, upscaled image
• Correct result: 400x300 cropped, upscaled image

[lilith]

Clarification for example 2, result 2: image should be cropped to 300x300, then padded with transparency to be 400x300.

[JaredReisinger]

I've added some DiagnosticJson regression tests for the above 3 cases, but I had to guess at the right numbers for the desired behavior. The current (master) code results in the following (the actual page has colors, which makes this easier to see):

autocrop scale down BAD (current):  passed
autocrop scale down GOOD (desired): failed : expected "300", got "400" for "result.finalRect.width".
autocrop scale down GOOD (desired): failed : expected "50", got "88" for "result.imageSourcePoly.y".
autocrop scale down GOOD (desired): failed : expected "300", got "225" for "result.imageSourcePoly.height".
autocrop scale down GOOD (desired): failed : expected "300", got "400" for "result.imageDestPoly.width".
autocrop scale down GOOD (desired): failed : expected "300", got "400" for "result.imageDestAreaPoly.width".

autocrop scale canvas BAD (current):  passed
autocrop scale canvas GOOD (desired): failed : expected "50", got "88" for "result.imageSourcePoly.y".
autocrop scale canvas GOOD (desired): failed : expected "300", got "225" for "result.imageSourcePoly.height".
autocrop scale canvas GOOD (desired): failed : expected "50", got "0" for "result.imageDestPoly.x".
autocrop scale canvas GOOD (desired): failed : expected "300", got "400" for "result.imageDestPoly.width".

autocrop scale both BAD (current):  passed
autocrop scale both GOOD (desired):  passed

manual/auto crop:  passed

Note that the BAD behavior is current passing, and the GOOD behavior is failing. (The third case @nathanaeljones listed had the current and correct values the same; I don't know if the current behavior is actually correct, or if there is a slight internal difference we should be looking for.) The fourth case ("manual/auto crop") comes from the comment in the pull request (issue #37), which would appear to already be passing.

@nathanaeljones, if you have a further set of commands/expectations in mind, I can add those as well; otherwise, I'll have to explore a bit to see what commands look like they might interact with autocrop and what values are interesting.

[lilith]

The current behavior is correct when scale=both is specified - the bug is for scale=down and scale=canvas being ignored.

[JaredReisinger]

Cool. I can flatten/merge the third regression case down to the One True Correct behavior.

Would you like me to go ahead and merge the pull request, review and test it, and commit/push if it looks good?

[lilith]

Yes, let's go ahead and merge it.