Simplify access to ImageState.Layout for unit testing.

[lilith]

See Plugins/Faces for one way to do this.

We need to drastically improve ImageResizer's unit test coverage, especially around Layout() work.

Oliver submitted a patch for one of our 2 outstanding bugs, but I'm afraid to merge it until we have better coverage for the rest of the features that interact with those mathematics.

Our current manual testing is... manual.

Can't wait until we can do layout in f#.

[JaredReisinger]

I'm not sure what in Plugins/Faces is a way that might simplify access to ImageState.Layout (LayoutBuilder), unless it's the return-JSON-instead-of-an-image code.

For unit testing Layout(), I'd imagine that you'd want a bunch of querystring variations crop/resize/padding/border/rotation/etc. on a sample image, with expectations about the resulting rings/bounding box on the LayoutBuilder. Does that sound right?

This seems like something that could go in ImageResizer.Core.Tests or ImageResizer.AllPlugins.Test (and probably the latter more than the former). I can start by creating some helpers that will make it easier to specify the expected LayoutBuilder/rings result, and then it's just a matter of building up enough tests against enough plugins to get the coverage we need.

[lilith]

Yes, I was referring to the JSON code.

By injecting a plugin that lets us provide input querystrings and original dimensions and get layout polygons as a result, we should be able to make our tests run against any 3.X build - or, with a bit more abstraction, anything with a matching REST api. Layout is a majority of the low-hanging fruit when it comes to integration tests.

In a future version, we'll probably replace the current Layout system with a more flexible approach, and we'll definitely want to be able to leverage our existing tests as we develop it. I think F# may offer us some benefits for this set of math-heavy tests, but it's up to you.

[JaredReisinger]

I've got a DiagnosticJson plugin far enough along that we'll want to settle on a "standard" for the layout information. My plan is to include the originalSize, destSize, and finalSize, and all of the "Ring" layout rings, including their names and points.

The JsonSerializer converts the sizes to comma-separated strings by default, and the points to a verbose format that includes things like the "IsEmpty" getter. For both Sizes and PointFs, we could serialize to a small object ({ "width":123, "height": 456 } or { "x":123, "y": 456 }), or we could serialize them as short, 2-element arrays. I'm leaning towards the former, because even though it's a bit more verbose, (a) it's intended only as a diagnostic, and (b) it's more self-descriptive.

My current JSON for this looks something like (spaces/newlines added only for readability):

{
"originalSize": {"width": 798, "height": 598},
"destSize": {"width": 300, "height": 225},
"finalSize": {"width": 300, "height": 225},

"copySize": {"width": 798, "height": 598},  // could be ignored, same data is in copyRect
"copyRect": {"x":0.0,"y":0.0,"width":798.0,"height":598.0},

"rings": [
    {"name":"image",
        "pointBehavior":"ClosestImagePoint",
        "points":[{"x":0.0,"y":0.0},{"x":300.0,"y":0.0},{"x":300.0,"y":225.0},{"x":0.0,"y":225.0}]},
    {"name":"imageArea",
        "pointBehavior":"ClosestImagePoint",
        "points":[{"x":0.0,"y":0.0},{"x":300.0,"y":0.0},{"x":300.0,"y":225.0},{"x":0.0,"y":225.0}]}
    ]
}

Does this feel right to you? Is there anything missing that you'd really want to see, or anything present that isn't really needed?

[lilith]

We probably need to figure out desired handing of sflip, srotate, and autorotate...I.e, how these affect originalSize and copyRect. Related: https://github.com/nathanaeljones/studiojs/blob/master/ImageResizer.js

If we make copyRect pre-rotate/flip, we need to make it a polygon. I think we might want to adopt a convention that includes both the polygon and the bounding rectangle of it (plus a flag if the bounding rectangle is 'exactly' the polygon). For all except a fraction of scenarios, the bounding rectangle is an exact match and easier to test against.

I.e, for any poly or ring, we could use this convention: {x:0,y:0,width:300,height:200, rect:true, points:[[0,0],[300,0],[300,200],[0,200]}

The solution might be to list both pre-rotate and post-rotate sourceSize and copyRect values...

sourceWidth
sourceHeight
sourceCopyPoly (optional)
adjustedSourceWidth
adjustedSourceHeight
adjustedCopyPoly (optional)
finalWidth
finalHeight
imageRenderPoly: {poly}
paddingRenderPoly: {poly}
translatedPoints: [[0,0],[1,1]]
resultInfo: { "result.ext": "jpg"...etc}
receivedInstructions: {"crop":"0,0,30,30"}

We would want to accept input points for translation as well, so that would make a 3rd input command.

Not all layout engines will necessarily adopt the rings approach either, so a flatter structure would help.

We should also echo the actual commands (parsed values re-serialized) that the layout engine actually sees.

[JaredReisinger]

You've been in the image-handling game longer than I, so I want to check my assumptions/conclusions against what you and ImageResizer users would expect. Conceptually, it seems like there are only 4 rects/polys that matter: the original source image rect (before any sFlip/sRotate), the final image rect (after all plugins add their changes), a poly (in source-rect coords) to represent the part of the original source image to copy, and a poly (in final-rect coords) to represent where the "to-copy" poly ends up. Any and all scaling, flipping, rotating, and cropping should be representable using those four values. (I am making the assumption that the order of the points in the polys matter, which is why they can include rotation information.)

I do see that ImageBuilder applies sFlip/sRotate early on (in PrepareSourceBitmap()) and modifies 'originalSize' in-place, which I think leads to your comment about figuring out their handling. Are you suggesting that in these cases we put the already-modified value into the adjustedSourceRect/adjustedCopyPoly fields and put back-calculated values into the sourceRect/sourceCopyPoly fields?

Finally, am I correct in understanding that the "image" ring is the destination poly (imageRenderPoly), and that the "imageArea" ring represents the constraint area (and is identical to "image" except when FitMode.Pad or ScaleMode.UpscaleCanvas kicks in)?

[lilith]

Conceptually you're completely correct.

Providing both original and adjusted values is, at some level, duplication, but I think it should make unit testing easier. Some low-level libraries don't offer any way to access sourceWidth/sourceHeight - they autorotate automatically, so you'd have to reverse-estimate based on leftover exif data. If you want to unit-test actual (vs. reverse-estimated) results, we'd want both values as well.

You are correct re: image and imageArea. Also, I was incorrect to call imageArea padding - they're separate rings (padding includes user-specified additional padding). imageAreaRenderPoly would be more correct.

[JaredReisinger]

Good to know I've got my head wrapped around it. Okay, here's some actual output from my latest iteration, using the query /red-leaf.jpg?sRotate=90&crop=10,20,100,200&diagnosticjson=layout. You can see that the requested crop is applied as 'adjustedSourceCopyPoly', and 'sourceRect' and 'sourceCopyPoly' have been back-calculated (and the copy poly is not a rect, indicating that there has been a rotation).

{
  "instructions": {
    "sRotate": "90",
    "crop": "10,20,100,200",
    "diagnosticjson": "layout"
  },
  "sourceRect": {
    "x": 0.0, "y": 0.0, "width": 798.0, "height": 598.0,
    "rect": true,
    "points": [ [ 0.0, 0.0 ], [ 798.0, 0.0 ], [ 798.0, 598.0 ], [ 0.0, 598.0 ] ]
  },
  "finalRect": {
    "x": 0.0, "y": 0.0, "width": 90.0, "height": 180.0,
    "rect": true,
    "points": [ [ 0.0, 0.0 ], [ 90.0, 0.0 ], [ 90.0, 180.0 ], [ 0.0, 180.0 ] ]
  },
  "sourceCopyPoly": {
    "x": 20.0, "y": 498.0, "width": 180.0, "height": 90.0,
    "rect": false,
    "points": [ [ 20.0, 588.0 ], [ 20.0, 498.0 ], [ 200.0, 498.0 ], [ 200.0, 588.0 ] ]
  },
  "imageRenderPoly": {
    "x": 0.0, "y": 0.0, "width": 90.0, "height": 180.0,
    "rect": true,
    "points": [ [ 0.0, 0.0 ], [ 90.0, 0.0 ], [ 90.0, 180.0 ], [ 0.0, 180.0 ] ]
  },
  "adjustedSourceRect": {
    "x": 0.0, "y": 0.0, "width": 598.0, "height": 798.0,
    "rect": true,
    "points": [ [ 0.0, 0.0 ], [ 598.0, 0.0 ], [ 598.0, 798.0 ], [ 0.0, 798.0 ] ]
  },
  "adjustedSourceCopyPoly": {
    "x": 10.0, "y": 20.0, "width": 90.0, "height": 180.0,
    "rect": true,
    "points": [ [ 10.0, 20.0 ], [ 100.0, 20.0 ], [ 100.0, 200.0 ], [ 10.0, 200.0 ] ]
  }
}

It seems a little weird for the 'sourceCopyPoly' value to sometimes be a rect and sometimes not... and when it's not, that implies that there are also going to be 'adjustedSource...' values. Because of the manner in which sFlip/sRotate/autorotate happen, would it be cleaner to make those back-calculated values the optional ones, so that 'sourceRect' and 'sourceCopyPoly' are always "normal"? (Maybe as 'rawSourceRect' and 'rawSourceCopyPoly'?)

You can see I don't have the 'imageAreaRenderPoly' in there yet, but that'll be trivial to add. Since you're also thinking about this as a general API, let me know if there are names other than 'imageRenderPoly' and 'imageAreaRenderPoly' that I should use. The names 'sourceCopyPoly' and 'imageRenderPoly' don't obviously pair, though the values do, but 'destCopyPoly' (and 'destAreaPoly'?) seem too vague. Maybe instead of 'sourceCopyPoly'/'imageRenderPoly'/'imageAreaRenderPoly', we should use 'imageSourcePoly'/'imageDestPoly'/'imageDestAreaPoly'?

[lilith]

I agree with all your naming suggestions. Also, I think for dimension-only size values, we don't need to follow the polygon syntax. Either flat nameWidth nameHeight or name: {width:value,height:value} works for me. Maybe a 'sourceImage' or 'rawImage' hash that could also contain additional useful data such as bitness, frame/page count, exif_rotate, and exif_flip values.

Reasoning about post-rotated values is certainly easier, so I would say that making those non-optional makes sense. Note that every pipeline has different characteristics about handling of rotation, and sometimes rotating the result will be much, much faster - if that path will produce a correct result. We don't yet have the flexibility to implement this kind of optimization, since we can't know if orientation-specific work (I.e, watermarking) is happening.

Note that if we add cropping rotation (I.e straightening), even the post-rotated rect would be rotated. (as only 90 deg intervals are usually handled during pre-rotation).

It's out of scope for now, but consider how we might extend this for testing more than one frame at a time. All frames of a GIF are the same size, but every page (or... tile!) of a TIFF file can be a different size and encoding. We don't support multi-page output now (you have to pick a page index) - but unit testing correct TIFF page size parsing could be useful, as we've had regressions in this area before. If there's a solution obvious to you, we might as well have the right data structure. If not, ignore this paragraph.

Image compositing (watermark) is yet another topic that's out of scope, yet has even more layout complexity.

[JaredReisinger]

I changed the dimension-only values to a simpler format... I could foresee value in having 'points' even for them (who knows how this might be used in the future), but it'll be easier to add new data than to remove it.

For additional data, and for GIF/TIFF-style pages, I could see adding a 'pages' array/object that contains more-or-less the same information for each page in the file. Again, adding additional information in the future will be easier than trying to account for it all now. Also, rather than 'diagnosticjson' being a true/false value, it's an enum (None/Layout, at present), which makes it easy to add values by asking for a different kind of 'diagnosticjson'. ("...?diagnosticjson=pages", for instance.