Transparent is not white

[NathanMOlson]

Fixes #26, by blending transparent pixels with a color that varies with position (instead of always blending transparent pixels with white).

[HarelM]

@mourner any chance to get this merged?
I think this is an elegant solution to this issue, it definitely improves the situation if not fully solves it.

[mourner]

Overall the change looks great! Just let me understand the color logic better and I think it'll be good to merge.

[mourner]

Hmm, left a comment here but it disappeared somewhere... Can you describe how these specific values / coefficients were picked, and maybe show a screenshot of how this looks?

[HarelM]

I think one of the tests shows that - for me it looked like how the color picker looks, and I guess the coefficients came from there...

[NathanMOlson]

This is what the background looks like:

There are a million choices for what the background image could look like. Here are my considerations when creating this background image:

1. The background should not be a uniform color.
2. The background should not contain large areas of uniform color.
3. The background should have a large amount of perceptual variability.
4. The background should be deterministic.
5. The background color should be easy to compute.
6. The background color should be a function of pixel index only (not x,y), because that is what was already available in colorDelta().
7. The background should not contain "common colors" (particularly white and black).
8. The background should not contain lines.
9. The background image should not contain anything we would expect to appear in test images.

[NathanMOlson]

The 48 and 159 were picked to give large color variability, while avoiding white, black, and fully saturated colors.

The 1.616 and 2.612 were picked to keep the pattern from degenerating into lines at certain integer image widths. I've added some digits to make sure theses coefficients preserve their irrationality for very large images :)

[mourner]

Amazing to see so much careful consideration going into this fix! Before we merge, the last thing left to do is to check whether and how much it affects performance — I don't expect there to be much overhead, but let's check just in case (JS perf can be weird sometimes).

[NathanMOlson]

Before we merge, the last thing left to do is to check whether and how much it affects performance

Here's the time in milliseconds for each unit test, as an average of 10 runs:

unit test #	main	transparent-is-not-white	difference
1	34.2	37.9	11%
2	18.9	20.5	9%
3	9.9	9.9	-1%
4	18.9	24.4	29%
5	12.0	12.6	5%
6	35.3	48.2	37%
7	13.0	8.5	-35%
8	8.8	11.0	25%
9	8.8	8.7	-1%
10	20.2	23.6	17%

[mourner]

@NathanMOlson a bit too variable to make a conclusion... e.g. could we try a median of 1000 runs after a warmup of 100 runs?

[NathanMOlson]

@NathanMOlson a bit too variable to make a conclusion... e.g. could we try a median of 1000 runs after a warmup of 100 runs?

Sure. Here you go. Another change I mage is to only profile the match calls instead of the whole test (which includes other expensive tasks like reading images).

unit test #	main	transparent-is-not-white	difference
1	1.671	3.319	99%
2	1.269	2.783	119%
3	0.240	0.239	0%
4	5.301	10.526	99%
5	1.485	2.589	74%
6	13.225	26.340	99%
7	0.901	1.349	50%
8	2.136	3.772	77%
9	0.563	0.372	-34%
10	4.205	6.649	58%

[NathanMOlson]

Since this is quite a bit slower, I did a run after changing the background color to

    const rBackground = 255;
    const gBackground = 255;
    const bBackground = 255;

Here's the results:

unit test #	main	transparent-is-not-white	difference	r=255, g=255, b=255	difference
1	1.671	3.319	99%	3.246	94%
2	1.269	2.783	119%	2.756	117%
3	0.240	0.239	0%	0.238	-1%
4	5.301	10.526	99%	9.972	88%
5	1.485	2.589	74%	2.565	73%
6	13.225	26.340	99%	24.997	89%
7	0.901	1.349	50%	1.330	48%
8	2.136	3.772	77%	3.595	68%
9	0.563	0.372	-34%	0.375	-33%
10	4.205	6.649	58%	6.400	52%

There is very little change between the variable background and the constant background, so I don't think we can expect performance gains from changing the background function. There's not much else changed in this PR, so my guess is that adding the third argument to blend() is responsible for the slowdown.

[NathanMOlson]

@NathanMOlson a bit too variable to make a conclusion... e.g. could we try a median of 1000 runs after a warmup of 100 runs?

@mourner You misunderstood the table. Each line is not a single test run, but an average of 10 runs for a single unit test. Each line is different because each unit test is different and takes a different amount of time. The variability is actually quite low.

However, the new tables (median from 1000 runs after 100 runs warmup) are better both because they have more runs and because they exclude the parts of the unit tests other than match().

[mourner]

@NathanMOlson I understood the table, just thought 10 runs aren't nearly enough to measure the difference reliably.

OK, since the difference is significant, let me play with this PR a bit more before we can merge — curious to find the culprit / way to optimize this.

[mourner]

Also, some of the tests don't even have semitransparent pixels, which makes it forego the blend calls altogether, which wouldn't produce such a difference, so this might be something with the benchmarking setup...

[NathanMOlson]

JS perf can be weird sometimes

I think this must be one of those cases. I've played around with it and haven't been able to improve it or get a clear understanding of what is causing the slowdown.

[NathanMOlson]

@mourner Any more thoughts about this?

[NathanMOlson]

@mourner Any update?

[NathanMOlson]

@mourner Any update?