Greatly improve capture_image_area on D2D, CG, and Cairo

[x3ro]

The capture_image_area method of the CoreGraphicsContext was not
taking into account that a transformation matrix is applied to the
context. As a result, it only worked correctly if one applied this
scaling manually to the src_rect prior to passing it to the method.

With this addition, we now respect the CG transformation matrix and
apply it to the src_rect prior to cropping the captured CGImage.

[xStrom]

Once the snapshot testing is properly updated in this PR, the proper target images will also need to be added to the submodule. You can read more about it in our CONTRIBUTING.md.

I'm not super familiar with that system, so @cmyr will probably need to step in here.

I did try this new test image out on Windows and the result does not look like the macOS result. So probably the Windows implementation is broken as well. Not sure about Linux. Well getting the sample count incremented should show us the situation a bit better, so let's start with that.

[x3ro]

Thanks for the feedback and the pointer to the snapshots. Out of curiosity, could you post the Windows image? I don't currently have access to a windows machine :)

[xStrom]

Direct2D result:

I also got curious and ran the Cairo code on Ubuntu. Found out that capture_image_area isn't implemented at all for Cairo.

[x3ro]

I just read through the CONTRIBUTING.md, and it seems like updating the reference snapshots is not particularly simple 😅 I'm curious what the reason is to not include these in the piet repository directly. Is this so that it's more difficult to unintentionally modify them? 🤔 If I understood correctly, someone would have to set up a branch for me in the piet-snapshots repository.

[x3ro]

Okay, so I took some time to also implement capture_image_rect for Cairo. At least on macOS (cairo 1.16.0) it behaves the same way as the coregraphics backend now. I'm putting this in this PR for now, but I'm happy to split it into a separate one, if that is desired.

I also have access to a Windows machine again, so I'll test the d2d behaviour as well soon-ish 😅

I also slightly changed the reference picture, so that it's a bit easier to spot if the cropping is slightly off, by using more prominent background colors. For reference, this is what I get at the moment:

[xStrom]

I'm curious what the reason is to not include these in the piet repository directly. Is this so that it's more difficult to unintentionally modify them? 🤔

I don't know the reason for sure, however I doubt it has to do with difficulity of changing them. My guess is that it is a way to prevent the main repository size from ballooning. Every iteration of the images would still be present in the git history. While a submodule can link to to a different repository with pruned history.

[x3ro]

Makes sense. In general I'm not in a hurry about getting this merged. However, I'm currently still kind of engaged in this topic, and usually my attention tends to drift after some point 😅 Is there anyone other than @cmyr who we could ping to help out here, or should we just wait until an answer comes along?

[xStrom]

There's Raph but I'll just message @cmyr directly to see if he can spare a moment, as he's definitely the top expert on this submodule snapshot business.

[cmyr]

@xStrom is right, the rationale for keeping these in a submodule is just so that there aren't a bunch of binary blobs in this repository, which would need to be downloaded on each checkout.

@x3ro I've invited you as a collaborator to that repo; let me know if you have any questions about how it all works :)

[x3ro]

Thanks, I'll try to get around to adding the snapshots in the coming days. In the meantime, maybe someone can help me out by looking at f24f897 – I tried to fix the test case for d2d, and also somehow got it to work, but something in that implementation is a bit fishy.

[xStrom]

The fact that it is 2.0 makes it seem like the scale (DPI) related code could be wrong. The test images are hard coded to use a scale of 2.0, which might be related. I have now opened a separate PR in #519 that at least lets us run the test image code with a few different scale factors, which might reveal bugs.

Unfortunately I ran out of time right now. However I plan on returning to this issue soon, perhaps tomorrow.

[x3ro]

Okay, so with the updated snapshots all tests pass now. As a next step I'm going to look into the d2d weirdness again 😅

[x3ro]

Cool, so this version seems to make sense overall, and passes the snapshots that I've created 🎉 The snapshots don't really test the transformation matrix behaviour beyond the dpi scaling (and, for d2d, not at all). Do we need to come up with test cases for that to get this merged?

[xStrom]

I keep finding bugs elsewhere in Piet when trying to test this PR. 😅 I've now submitted #520 to fix an issue that caused the macOS test picture generator to crash at any scale other than 2.0.

There's also another issue related to pixel components that I'm still investigating, but will likely open a PR for, tomorrow.

Hopefully then I can finally get a better grasp on what this PR here is doing, and provide some feedback.

[xStrom]

I ran the test image with 1x scale instead of the default 2x scale and got weird results.

CoreGraphics 1x scale

Looks the most correct, although I haven't measured anything.

Cairo 1x scale

The corner boxes seem to be missing their red lines.

Direct2D 1x scale

There are even more red lines missing, and the whole thing is shifted to the top left corner.

I haven't had time yet to actually try to figure out what is causing these issues, but I thought it's worth reporting the findings.

Instead I've been trying to improve the whole test picture system. #519, #520, #521 are now merged. #522 isn't merged yet, but once it is it will provide an easier way to generate these test images at different scale factors. Otherwise if you rebase on master right now then you need to modify SCALE in test-picture.rs.

[x3ro]

Thanks for your work on the test picture system and for sharing your findings. I'll look into why this breaks w/ d2d and cairo :)

[x3ro]

Mmh, I've looked at Cairo, and there it could just be a rounding problem. The boxes are

let outer_rect_red = Rect::new(20., 20., 180., 180.);
let inner_rect_blue = Rect::new(21., 21., 179., 179.);

With 2x scale, this results in a 1px thin line, and thus with 1x scale it should be a 0.5px thin line, which ends up not being visible 🤔 At least that is my guess. Making the line 2px thick in 2x scale makes it visible in 1x scale as well.

[jneem]

You're doing InterpolationMode::NearestNeighbor, and with that scaling there are two different neighbors (one red, one blue) at exactly the same distance, so maybe the outcome just depends on how the rounding works to determine the nearest neighbor? Does InterpolationMode::Biliear give a different result?

[x3ro]

InterpolationMode::Biliear, at least for the Cairo backend, had the same result, i.e. no visible border. Increasing the border size to 2px did solve the issue on the Cairo side (this is already pushed). Have yet to look into d2d.

[xStrom]

I finally had some time to dig deeper into the Direct2D issues. I think I solved them and have written about the changes needed in this review comment batch.

I'll still need to review CoreGraphics and Cairo later too, but hopefully we're getting close to done here.

[x3ro]

Nice, thanks so much for your thorough review. I'll see when I get around to this, but I won't have access to a Windows machine until at least next week, so I guess it'll happen then 😅

[xStrom]

You could make those changes on a non-Windows machine. I'm pretty confident it will work and can do the testing myself, plus CI.

[x3ro]

It does indeed look correct now, thanks for the fix! I think I'll get around to updating the snapshots tomorrow 🚀

[x3ro]

Looks like this is working as intended now. So now I would merge the snapshots, and then we'd merge this PR? 🤔

[cmyr]

order doesn't really matter but normally I'd get the PR in first, then merge the snapshots. The snapshots will continue to work as long as the referenced commit exists, it doesn't need to be in the main branch. 🤷

[xStrom]

Yeah, I'll have time tomorrow to do the final review here and then merge. After this is merged, we can merge snapshots.

[xStrom]

Thank you for your continued effort to get this done! I have tested it on all three platforms and it seems to be correct.

The Affine transformations aren't tested, but that is a fine line to split the work and if someone runs into issues with it, it can be resolved in a different PR.