Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

make annotation tests strict #3083

Merged
merged 5 commits into from
Nov 26, 2015
Merged

make annotation tests strict #3083

merged 5 commits into from
Nov 26, 2015

Conversation

jfirebaugh
Copy link
Contributor

The annotation tests currently require manual verification. Predictably, they regressed without me noticing (my fault).

We need to:

  • Port pixelmatch to C++ (pixelmatch-cpp?) /cc @mourner
  • Use it to verify expected rendering in the annotation tests (and possibly elsewhere)

@mourner
Copy link
Member

mourner commented Nov 20, 2015

Do we have to test in C++? The test suite for Native runs in JS after all.

@jfirebaugh
Copy link
Contributor Author

Yes, we don't want to write node bindings for every internal thing we test.

@mikemorris
Copy link
Contributor

We would likely need #2161 before we could test annotations through Node @mourner.

@jfirebaugh jfirebaugh self-assigned this Nov 21, 2015
@jfirebaugh
Copy link
Contributor Author

Ok, pixelmatch is ported and in use. Tests are passing locally by failing on CI. I think it's something to do with PNG color profiles?

Here's a sample failure.

Expected:
image

Actual:
image

Diff:
image

When I run the JS version of pixelmatch locally on those files, it reports they are identical.

@mourner Have you encountered this?

@incanus
Copy link
Contributor

incanus commented Nov 21, 2015

Color profiles seems likely.

@jfirebaugh jfirebaugh force-pushed the pixelmatch branch 8 times, most recently from 6fdf3d2 to a5d1b9a Compare November 25, 2015 23:26
* Consolidate Image and StillImage
* Typecheck unassociated vs premultiplied images
* Rewrite default platform image decoding implementation
It appears to be the only way to get the results we want in all cases.
It's a premultiplied image. This implies that we were misusing encodePNG
in most cases, as we were passing premultiplied pixels which were then
interpreted as unmultiplied. I changed encodePNG to accept premultipled
pixels, and unpremultiply in the implementations.
@mourner
Copy link
Member

mourner commented Nov 26, 2015

Ok, pixelmatch is ported and in use. Tests are passing locally by failing on CI. I think it's something to do with PNG color profiles?

@jfirebaugh what was it? Haven't encountered anything similar on JS side.

@jfirebaugh
Copy link
Contributor Author

It was undesired premultiplication behavior of libpng when a color profile is present in the source PNG, plus confusion in our code about where image data is premultiplied and where is is not. To fix the first, I disabled all premultiplication in libpng and manually premultiply using an algorithm that is reversed engineered to produce the same results as Core Graphics (premultiplication algorithms are surprisingly subtle). To fix the second, I rewrote most imaging code to work with explicit PremultipliedImage and UnassociatedImage (non-premultiplied) types. We should be using PremultipliedImage more or less everywhere, the exception being at the very edge of reading/writing PNGs, which are always unpremultiplied.

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

Successfully merging this pull request may close these issues.

4 participants