Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for optimised reference image PNGs #80

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukaskukacka
Copy link

TL;DR: Don't fail test / comparison if recorded saved reference image PNG is optimized using tools like ImageOptim.

Overview

This allows to use apps like ImageOptim.app to optimise recorded reference images. Optimising reference image can heavily reduce its size, meaning we need to store less binary data in repo.

Technical details

The core of this change is the way how CGContextRef are initialized.

Before (original code)

Each context was using minBytesPerRow to calculate buffer size. However, this was reason of the failure in case of optimized images. ImageOptim will for example reduce color space of the PNG from 32bit to 8bit (monochromatic) where possible. This results in minBytesPerRow being wrong value (not big enough to fit runtime rendered snapshot).

After (this change)

Contexts are always built based on "bigger" (the one needing more memory) context of the two images (saved reference vs rendered). This means context will always be built based on runtime rendered image. When rendering CGContextDrawImage to context, both runtime snapshot and loaded optimized reference images are "normalized" to the same context (colorspace etc.).

Backwards compatibility

✅ 100% backwards compatible

Optimizing reference images is opt-in, optional and fully backwards compatible. This PR will not impact existing projects which don't use optimized reference images.

Performance impact

✅ None (negligible)

  • No runtime impact
    • Comparison code is not impacted.
    • Optimizations like memcmp works the same.
  • No (negligible) memory impact
    • Rendering contexts to memory buffers is the same. Change Both contexts are mostly rendered using 4x8bits per pixel.
    • The only impact is the removal of the minBytesPerRow padding optimization, which should have almost no impact.

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2019

CLA assistant check
All committers have signed the CLA.

@rudro rudro requested a review from reidmain May 22, 2019 00:07
@lukaskukacka
Copy link
Author

lukaskukacka commented Jan 3, 2020

@reidmain is anyone still maintaining this project / any chance to get this merged please?

@lukaskukacka lukaskukacka force-pushed the OptimisedReferenceImagesSupport branch 3 times, most recently from ee17d57 to 42f6e2d Compare January 3, 2020 14:31
@lukaskukacka
Copy link
Author

@reidmain Any chance for this to being merged? Is the project still being maintained please? PRs seems to be abandoned (this one is opened 10 months with no response)

Copy link

@reidmain reidmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can no longer approve any changes to this repository. I'm going to request changes to this PR so it can be removed from my queue.

@alanzeino
Copy link
Collaborator

How does this compare to #140?

@alanzeino alanzeino self-requested a review August 25, 2021 21:40
@lukaskukacka
Copy link
Author

Thanks @alanzeino for having a look.

I believe both this and #140 have the same goal and almost identical implementation. This was just waiting and ready here 1.5y before #140 😀

Looking at the code the solution is almost the same as well.
The main difference seems to me that this PR uses color space based on reference image ( CGImageGetColorSpace(contextConfigImage.CGImage)) while #140 hardcodes color space (CGColorSpaceCreateDeviceRGB()), which I am not sure is safe to do.

// Find image which requires more bytes in memory. We have to normalise both images to context requiring "bigger" memory representation.
// This allows comparing 2 visually identic images even if their bit representation in file can be different
// (for example reference images optimised using ImageOptim app).
// Because both images have the same pixel size, image with more bytes per row is the image dictating the context confix for comparison.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: config

Suggested change
// Because both images have the same pixel size, image with more bytes per row is the image dictating the context confix for comparison.
// Because both images have the same pixel size, image with more bytes per row is the image dictating the context config for comparison.

size_t minBytesPerRow = MIN(CGImageGetBytesPerRow(self.CGImage), CGImageGetBytesPerRow(image.CGImage));
size_t referenceImageSizeBytes = referenceImageSize.height * minBytesPerRow;
// Find image which requires more bytes in memory. We have to normalise both images to context requiring "bigger" memory representation.
// This allows comparing 2 visually identic images even if their bit representation in file can be different

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: identical

Suggested change
// This allows comparing 2 visually identic images even if their bit representation in file can be different
// This allows comparing 2 visually identical images even if their bit representation in file can be different

This allows to use apps like ImageOptim.app to optimise recorded
reference images. Optimising reference image can heavily reduce
its size, meaning we need to store less binary data in repo.
@lukaskukacka lukaskukacka force-pushed the OptimisedReferenceImagesSupport branch from 42f6e2d to 3fa343c Compare August 30, 2021 21:07
@bernikovich
Copy link

Hey. I faced the same problem as the author and ended up with the same fix. The only concern is that it may fail when one image has more bits per component while the other has more bits per pixel and bytes per row. But this is probably very rare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants