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.

Sign up for GitHub

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

Jump to bottom

lanczos3: Scale input kernel by source size, not target size #26

Merged
merged 1 commit into from
Aug 23, 2023
Merged

lanczos3: Scale input kernel by source size, not target size #26

merged 1 commit into from
Aug 23, 2023

Conversation

MarijnS95
Copy link
Member

When the 7x7 kernel runs over pixels in the source image, its offset is multiplied by the inverse of the target image size to turn these integer coordinates into float coordinates (only to turn them back into integer on the next line 🤦). This means that the floating-point offset of the kernel relative to the center of the source pixel we are sampling is now relative to the dimensions of the target image instead of the source image, leading us to skip source pixels as the target is smaller than the source and thus the offset is larger than one step in source-pixels.

Take for example the Dalai Lama test picture from http://www.ericbrasseur.org/gamma.html?i=1 with alternating green and pink lines. When downsampling it 4 times using our examples/test.rs only pink (or green) lines are sampled consecutively, highlighting that we are skipping the pixel in-between entirely:

square_test_result

After this PR, all pixels are properly sampled:

square_test_result

This leaves the image grey with some artifacts, highlighting the linearization issues pointed out in #25.

Note that I am still completely unsure if the same fix should be applied to when we calculate the "center pixel" which is also converted to be relative to the target image:

ispc-downsampler/src/ispc/kernels/lanczos3.ispc

Lines 55 to 59 in dff71fb

float<2> center_pixel = uv * target_size;
center_pixel.x = frac(center_pixel.x);
center_pixel.y = frac(center_pixel.y);
float<2> center = uv - (center_pixel - 0.5) * inv_target_size;
float<2> offset = (uv - center) * target_size;

An alternative fix is to revisit this code and convert it to compute in integer space instead.

MarijnS95 added a commit that referenced this pull request Aug 23, 2023
@MarijnS95
lanczos3: Const-fold offset calculation to hardcoded 0.5
a9cf272 
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.`.
@MarijnS95 MarijnS95 mentioned this pull request Aug 23, 2023
Merged
@MarijnS95
Copy link
Member Author

Note that I am still completely unsure if the same fix should be applied to when we calculate the "center pixel" which is also converted to be relative to the target image:

Nope, but it could be drastically simplified. Understanding what this calculation does lead me to #28.

@MarijnS95
lanczos3: Scale input kernel by source size, not target size
f163a86 
When the 7x7 kernel runs over pixels in the _source_ image, its offset
is multiplied by the inverse of the _target_ image size to turn these
integer coordinates into float coordinates (only to turn them back into
integer on the next line 🤦). This means that the floating-point
offset of the kernel relative to the center of the source pixel we are
sampling is now relative to the dimensions of the target image instead
of the source image, leading us to skip source pixels as the target is
smaller than the source and thus the offset is larger than one step in
source pixels.
MarijnS95 added a commit that referenced this pull request Aug 23, 2023
@MarijnS95
lanczos3: Const-fold offset calculation to hardcoded 0.5
0d417d3 
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.`.
@MarijnS95
Copy link
Member Author

Note that this variable is once again removed in #29, as it turns out to be unnecessary (who'd have thought) after using integer space math as suggested above.

@MarijnS95 MarijnS95 merged commit e545343 into main Aug 23, 2023
10 checks passed
@MarijnS95 MarijnS95 deleted the lanczos3-kernel-range branch August 23, 2023 13:00
Athosvk pushed a commit that referenced this pull request Aug 23, 2023
@MarijnS95
lanczos3: Const-fold offset calculation to hardcoded 0.5 (#28)
4164ee3 
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.`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Athosvk Athosvk Athosvk approved these changes

@KYovchevski KYovchevski Awaiting requested review from KYovchevski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MarijnS95 @Athosvk