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

blur[12] #252

Merged
merged 13 commits into from
Jul 2, 2022
Merged

blur[12] #252

merged 13 commits into from
Jul 2, 2022

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jun 30, 2022

Demo 1: https://observablehq.com/d/a0492676582ff10a
Demo 2: https://observablehq.com/d/49e07886c2dbaacb

In this PR:

  1. Separate interfaces for one- and two-dimensional blurring
  2. Additional interface for blurring (RGBA) ImageData
  3. Implement fractional rolling window size for better fractional radius
  4. Implement arbitrary step (stride) rolling window (e.g., for blurring RGBA ImageData)
  5. Inline three-pass blur instead of for loop
  6. Blur in-place instead of copying (and dealing with iterables etc.)
  7. Create only a single copy of the input array Use only a small circular buffer
  8. Positional arguments instead of D3-style method chaining
  9. Stricter validation of options (and use Math.floor instead of Math.round per language idiom)
  10. Use Math.* directly instead of importing extracted symbols

I haven’t actually implemented RGBA ImageData blurring yet, but it should be possible to use the blurf function here to do that. (I’d love a contribution to this PR!) Added blurImage and blurfImage.

@mbostock mbostock requested a review from Fil June 30, 2022 12:50
@mbostock mbostock changed the title sketch blur[12] blur[12] Jul 1, 2022
@mbostock mbostock marked this pull request as ready for review July 1, 2022 01:52
src/blur.js Outdated Show resolved Hide resolved
src/blur.js Outdated Show resolved Hide resolved
@curran
Copy link

curran commented Jul 1, 2022

How does this compare to #151?

@Fil
Copy link
Member

Fil commented Jul 1, 2022

How does this compare to #151?

The API is different:

  1. you don't have setters and getters for the options, but just a direct function call.
  2. The array is blurred in-place and returned, so you have to be careful and do B = blur1(A.slice(), 3)
  3. There are two signatures blur1 and blur2, depending on whether you want 1-d or 2-d blurring.

Fractional blurring (for example, r = 1.5) is much more balanced (see Mike's demo)

In terms of performance the difference is impressive: Mike's new implementation is 6 to 8 times faster than mine in 1-d [tests]; similar results on macos Chrome, Firefox and Safari.

I'm surprised both by the huge performance boost in 1-d and the fact that it doesn't result in hugely better performance in 2-d (“only” 30% faster—which is already a big win). Maybe what's slowest is the number of copies?

I think I'd want blur1 to be renamed blur, with a default r = 5, so I can write blur(A).

@Fil
Copy link
Member

Fil commented Jul 1, 2022

#254 adds the stride.

@mbostock
Copy link
Member Author

mbostock commented Jul 1, 2022

I think I’ve got it working with a circular buffer of size 2 * radius + 1, eliminating the need to copy the entire input values array. That should make the performance even better. I’ll push more changes later today.

@mbostock
Copy link
Member Author

mbostock commented Jul 1, 2022

How does this compare to #151?

I think this is pretty well covered in the top description? If you have more constructive questions, please ask.

@curran
Copy link

curran commented Jul 1, 2022

My aim with the question was to illuminate how this implementation compares and contrasts with the other. The top description describes this implementation only, without any comparisons or references to the other, so at first glance it was not clear what advantages this implementation has over the other. Thank you @Fil for thoroughly addressing my query - that's exactly the kind of information I was looking for. Thank you both for your amazing work!

@mbostock
Copy link
Member Author

mbostock commented Jul 1, 2022

The top description describes this implementation only, without any comparisons or references to the other

I strongly disagree; the description enumerates the changes in this PR versus the other branch.

@curran
Copy link

curran commented Jul 1, 2022

Ooooohhhh! Sorry, I didn't realize that. My mistake. Thank you for clarifying.

@Fil
Copy link
Member

Fil commented Jul 1, 2022

Here's a benchmark https://github.com/d3/blur-benchmark

Overall the circular buffer is quite faster on larger arrays, but a bit slower on medium-sized arrays.

@mbostock
Copy link
Member Author

mbostock commented Jul 1, 2022

I’m seeing that the circular buffer is quite slower than the copy in Chrome, so given the added complexity of this approach I’m going to unwind the latest commit and stick with the original approach.

I still want to think about strides and RGBA blurring but otherwise this PR is ready for review.

@mbostock
Copy link
Member Author

mbostock commented Jul 2, 2022

I wrote an adapter to make the blur functions work for RGBA image data:

function blurfImage(radius) {
  const blur = blurf(radius);
  return (T, S, start, stop, step) => {
    start <<= 2, stop <<= 2, step <<= 2;
    blur(T, S, start + 0, stop + 0, step);
    blur(T, S, start + 1, stop + 1, step);
    blur(T, S, start + 2, stop + 2, step);
    blur(T, S, start + 3, stop + 3, step);
  };
}

Otherwise the algorithm is the same. Hooray!

@mbostock mbostock mentioned this pull request Jul 2, 2022
@mbostock
Copy link
Member Author

mbostock commented Jul 2, 2022

I’m quite happy with how this turned out. I hope you like it, @Fil! 😅

Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

Yay! It's the fastest in all tests (one exception is the "2D huge" test).

I'd like to rename blur1 to blur, have r default to 5, and make height optional since it's redundant information.

Also, it seems this version doesn't support iterables… is it something we might want to add back (with arrayify?).

@mbostock
Copy link
Member Author

mbostock commented Jul 2, 2022

I'd like to rename blur1 to blur, have r default to 5, and make height optional since it's redundant information.

Hmm. I’m okay with the rename.

I don’t see a lot of value in having a default for r; when would you want to blur with a default radius, and why is 5 the right default? The CSS blur doesn’t have a default. https://developer.mozilla.org/en-US/docs/Web/CSS/filter-function/blur

Making height redundant seems okay, although there’s something to be said for being explicit.

Also, it seems this version doesn't support iterables… is it something we might want to add back (with arrayify?).

No; you can’t modify an iterable in-place as you need to be able to assign indexed values, not just iterate over them. If you have an iterable, you’d need to convert it into an array first yourself.

@Fil
Copy link
Member

Fil commented Jul 2, 2022

when would you want to blur with a default radius

When I compute the laplacian of a slow moving value (no high frequency), the radius is just a scaling factor. Not a big issue.

@Fil Fil merged commit f798c25 into blur Jul 2, 2022
@Fil Fil deleted the mbostock/blur branch July 2, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants