Fix #124842: Fix CPU compositor sampling errors and make it match GPU #125149

Bill-Spitzak · 2024-07-21T01:00:21+02:00

Bill-Spitzak commented

2024-07-21 01:00:21 +02:00

Fixes a number of errors with cropping and with 0.5 offsets in the coordinates. Some code cleanup and reuse.

Bill-Spitzak requested review from Omar Emara 2024-07-21 01:00:54 +02:00

Omar Emara requested changes 2024-07-22 10:04:06 +02:00

Omar Emara left a comment

Your patch seems to include many unrelated changes that are mostly unrelated to #124842. So it is unlikely to be merged in its current form. The patch is also undercounted and the description is lacking the necessary information for review.
See https://developer.blender.org/docs/handbook/contributing/.

I would recommend you only change the minimal amount of code to fix the issue with clear patch description on what the problem is and how you solved it.

Your patch seems to include many unrelated changes that are mostly unrelated to #124842. So it is unlikely to be merged in its current form. The patch is also undercounted and the description is lacking the necessary information for review. See https://developer.blender.org/docs/handbook/contributing/. I would recommend you only change the minimal amount of code to fix the issue with clear patch description on what the problem is and how you solved it.

source/blender/blenlib/intern/math_interp.cc

						
				@ -218,2 +218,4 @@

				/* border = false: Extend if wrap is false

				 * border = true:  Clip if wrap is false */

				template<bool border>

				BLI_INLINE void bilinear_fl_impl(const float *buffer,

Omar Emara commented

2024-07-22 09:49:09 +02:00

What are the changes in this function supposed to do?

Bill-Spitzak commented

2024-07-23 00:25:06 +02:00

These are just code cleanup from an aborted attempt to make the same changes to the bicubic sampler. I gave up on that as being too intrusive, in particular because the bicubic one always does Extend. Can remove them if desired.

source/blender/compositor/intern/COM_MemoryBuffer.h Outdated

						
				@ -252,50 +252,7 @@ class MemoryBuffer {

				  void read_elem_bilinear(float x, float y, float *out) const

				  {

				    /* Only clear past +/-1 borders to be able to smooth edges. */

Omar Emara commented

2024-07-22 09:54:45 +02:00

What are the functional changes associated with all of those changes?

Bill-Spitzak commented

2024-07-23 00:27:49 +02:00

The Extend version of the samplers is used (except for Bilinear which will use Wrap when requested because it was doing it correctly). Clip is done by here so it can be enabled per-axis and it performs the correct calculation, Extend is passed to the samplers. This did require significant code rearrangement as I did not want to change the behavior of the existing per-sampler calls, since they are used elsewhere in COM.

The `Extend` version of the samplers is used (except for Bilinear which will use Wrap when requested because it was doing it correctly). Clip is done by here so it can be enabled per-axis and it performs the correct calculation, Extend is passed to the samplers. This did require significant code rearrangement as I did not want to change the behavior of the existing per-sampler calls, since they are used elsewhere in COM.

source/blender/compositor/operations/COM_RotateOperation.cc Outdated

						
				@ -64,3 +63,1 @@

				  r_bounds.xmax = ceil(maxx);

				  r_bounds.ymin = floor(miny);

				  r_bounds.ymax = ceil(maxy);

				  r_bounds.xmin = floor(minx + 0.1f);

Omar Emara commented

2024-07-22 09:55:51 +02:00

Why those biases?

Bill-Spitzak commented

2024-07-23 00:33:26 +02:00

This is a mistake, they only fixed the Scale. It appears the errors in Rotate are much larger but I also think it is less vital to fix them.

source/blender/compositor/operations/COM_ScaleOperation.cc Outdated

						
				@ -167,3 +167,3 @@

				    const float rel_scale_y = *it.in(1) * scale_y_factor;

				    const float scaled_x = scale_coord_inverted(

				        from_scale_offset_x + canvas_.xmin + it.x, scale_center_x, rel_scale_x);

				        from_scale_offset_x + canvas_.xmin + 0.5f + it.x, scale_center_x, rel_scale_x) - 0.5f;