-
Notifications
You must be signed in to change notification settings - Fork 1
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
lanczos3: Const-fold offset
calculation to hardcoded 0.5
#28
Conversation
a43d119
to
f163a86
Compare
a9cf272
to
0d417d3
Compare
0d417d3
to
d753ad3
Compare
Note that this PR basically shows that the filter kernel is constant for the given dimensions. This idea was also implemented in (the massively conflated, IMO) PR #15. Apparently all this broken code that I'm fixing with all these PRs has been completely removed there too; we should've put more effort into cleaning it up and landing it. But it's also useful to have a fixed target implementation to compare to. |
The usefulness of this magic took a fair bit of time to understand, while we can trivially remove it after deducing that it always computes to the constant `0.5`, and gets rid of some strange bright spots in the center of our image compared to #26. Before: ![square_test_result](https://github.com/Traverse-Research/ispc-downsampler/assets/2325264/273556b4-6f53-43d5-9424-31fef5ca7966) After: ![square_test_result](https://github.com/Traverse-Research/ispc-downsampler/assets/2325264/e0507eee-9a58-4fd8-b9fc-7a0a3c485ee8) First, we start by knowing that `uv` is divided by `target_size` before it is passed to `resample_internal()`. Hence, if we multiply it by `target_size` again, there should be no fractional part and `center_pixel` always becomes `0`. Floating point rounding errors being gone now, this is what solves the bright spots in the center of the image mentioned above. Then we are left with: center = uv - (0.0 - 0.5) * inv_target_size Which becomes: center = uv + 0.5 * inv_target_size As a drive-by cleanup we can now see that `(inv_)target_size` is only used to offset `uv` by another half _target_ pixel to point to the center instead of the top-left. These values were already involved in converting the `uv` coordinate from target pixels to normalized coordinates, so it reads more logical (involving less math) to factor this calculation into the call site and remove two extraneous function parameters from `resample_internal()` as a result. Now, continuing our journey, plug this into `offset` and simplify: offset = (uv - center) * target_size offset = (uv - (uv + 0.5 * inv_target_size)) * target_size offset = (-0.5 * inv_target_size) * target_size offset = -0.5 And we have our target value. Then, because they are subtracted when calling `lanczos3_filter()`, we turn this into positive `0.5`. Note that I have _zero_ clue whether this is the right value, but when sampling a 6x6 grid (not 7x7 as thought in #27) we only visit pixel positions `[-3, ..., 2]`, thus neatly retrieving weights at `[-2.5, ..., 2.5]` and never hitting the `3.5` value which is above `3` where `lanczos3_filter(3.5)` returns `0.`.
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?
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?
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?
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?
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?
Depends on #26
The usefulness of this magic took a fair bit of time to understand, while we can trivially remove it after deducing that it always computes to the constant
0.5
, and gets rid of some strange bright spots in the center of our image compared to #26.Before:
After:
First, we start by knowing that
uv
is divided bytarget_size
before it is passed toresample_internal()
. Hence, if we multiply it bytarget_size
again, there should be no fractional part andcenter_pixel
always becomes0
. Floating point rounding errors being gone now, this is what solves the bright spots in the center of the image mentioned above.Then we are left with:
Which becomes:
As a drive-by cleanup we can now see that
(inv_)target_size
is only used to offsetuv
by another half target pixel to point to the center instead of the top-left. These values were already involved in converting theuv
coordinate from target pixels to normalized coordinates, so it reads more logical (involving less math) to factor this calculation into the call site and remove two extraneous function parameters fromresample_internal()
as a result.Now, continuing our journey, plug this into
offset
and simplify:And we have our target value. Then, because they are subtracted when calling
lanczos3_filter()
, we turn this into positive0.5
.Note that I have zero clue whether this is the right value, but when sampling a 6x6 grid (not 7x7 as thought in #27) we only visit pixel positions
[-3, ..., 2]
, thus neatly retrieving weights at[-2.5, ..., 2.5]
and never hitting the3.5
value which is above3
wherelanczos3_filter(3.5)
returns0.
.