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

Revert "lanczos3: Actually sample 7x7 instead of 6x6 (#27)" #31

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

MarijnS95
Copy link
Member

This reverts commit 22e1bb1.

The kernel size of the lanczos3 filter is 6x6, and sampling it at x=3.5 or y=3.5 results in a weight of 0, thus making these pixels completely irrelevant. This became more clear in #28 that simplified the offset passed to lanczos3_filter() to always be 0.5, to read the weight at the middle of each source pixel.

Note that for an even reduction in image size the center coordinate of every target pixel (what uv denotes) is exactly on the boundary between two source pixels, meaning the pixel at kernel position x=0,y=0 (barring float imprecisions) is at the right/bottom of the center of the target pixel, hence correctly reading 3 pixels to the left, top, right and bottom (with indices in the range [-3, 2]).

For uneven reductions (i.e. 3x) this doesn't hold, and that was likely what the code removed in #28 was incorrectly trying to compensate for?

@MarijnS95
Copy link
Member Author

For uneven reductions (i.e. 3x) this doesn't hold, and that was likely what the code removed in #28 was incorrectly trying to compensate for?

@KYovchevski can you shed some light on this? Perhaps we should add part of that code back to make it work again?

@MarijnS95
Copy link
Member Author

The kernel size of the lanczos3 filter is 6x6, and sampling it at x=3.5 or y=3.5 results in a weight of 0, thus making these pixels completely irrelevant

Quite funky that reading 7x7 - 6x6 = 13 more pixels per iteration (and weighting them with 0) has no effect on cargo bench timings.

This reverts commit 22e1bb1.

The kernel size of the `lanczos3` filter is 6x6, and sampling it at
`x=3.5` or `y=3.5` results in a weight of `0`, thus making these pixels
completely irrelevant.  This became more clear in #28 that simplified
the offset passed to `lanczos3_filter()` to always be `0.5`, to read the
weight at the middle of each source pixel.

Note that for an even reduction in image size the center coordinate
of every target pixel (what `uv` denotes) is exactly on the boundary
between two source pixels, meaning the pixel at kernel position
`x=0,y=0` (barring float imprecisions) is at the right/bottom of the
center of the target pixel, hence correctly reading 3 pixels to the
left, top, right and bottom (with indices in the range [-3, 2]).

For uneven reductions (i.e. 3x) this doesn't hold, and that was likely
what the code removed in #28 was incorrectly trying to compensate for?
@MarijnS95
Copy link
Member Author

We should really merge this. Before:

Downsample `square_test.png` using ispc_downsampler
                    time:   [54.067 ms 54.110 ms 54.154 ms]

After:

Downsample `square_test.png` using ispc_downsampler
                    time:   [43.947 ms 44.037 ms 44.175 ms]
                    change: [-18.798% -18.615% -18.374%] (p = 0.00 < 0.05)
                    Performance has improved.

@MarijnS95 MarijnS95 merged commit c21b5db into main Oct 23, 2023
10 checks passed
@MarijnS95 MarijnS95 deleted the revert-7x7-kernel branch October 23, 2023 20:06
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.

None yet

1 participant