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

Transparent is not white #142

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

NathanMOlson
Copy link

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

@HarelM
Copy link

HarelM commented Oct 21, 2024

@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.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

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

index.js Outdated

const rBackground = 48 + 159 * (k % 2);
const gBackground = 48 + 159 * (Math.floor(k / 1.616) % 2);
const bBackground = 48 + 159 * (Math.floor(k / 2.612) % 2);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link

@HarelM HarelM Oct 21, 2024

Choose a reason for hiding this comment

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

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...

Copy link
Author

@NathanMOlson NathanMOlson Oct 21, 2024

Choose a reason for hiding this comment

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

This is what the background looks like:
background

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 :)

index.js Outdated Show resolved Hide resolved
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

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
Copy link
Author

NathanMOlson commented Oct 22, 2024

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
Copy link
Member

mourner commented Oct 22, 2024

@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
Copy link
Author

NathanMOlson commented Oct 22, 2024

@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
Copy link
Author

NathanMOlson commented Oct 22, 2024

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
Copy link
Author

@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
Copy link
Member

mourner commented Oct 22, 2024

@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
Copy link
Member

mourner commented Oct 22, 2024

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
Copy link
Author

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
Copy link
Author

@mourner Any more thoughts about this?

@NathanMOlson
Copy link
Author

@mourner Any update?

1 similar comment
@NathanMOlson
Copy link
Author

@mourner Any update?

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.

Detect difference between white and transparent pixels
3 participants