lanczos3: Actually sample 7x7 instead of 6x6 #27
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`<` should have been `<=`, otherwise we only sample `[-3, -2, -1, 0, 1, 2]` biasing us towards the top-left of the pixels.
EmilioLaiso
approved these changes
Aug 23, 2023
|
|
I am not actually sure if it is intended that we sample a 6x6 grid from |
MarijnS95
added a commit
that referenced
this pull request
Aug 23, 2023
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 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.`.
MarijnS95
added a commit
that referenced
this pull request
Aug 23, 2023
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 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.`.
Athosvk
pushed a commit
that referenced
this pull request
Aug 23, 2023
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 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.`.
MarijnS95
added a commit
that referenced
this pull request
Aug 25, 2023
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
added a commit
that referenced
this pull request
Aug 26, 2023
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
added a commit
that referenced
this pull request
Oct 23, 2023
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
added a commit
that referenced
this pull request
Oct 23, 2023
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
added a commit
that referenced
this pull request
Oct 23, 2023
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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
<should have been<=, otherwise we only sample[-3, -2, -1, 0, 1, 2]biasing us towards the top-left of the pixels.