Re: [Pixman] [PATCH v13 08/14] pixman-filter: Corrections to the integral() function

[Bill Spitzak]

On Fri, Mar 4, 2016 at 3:12 PM, Søren Sandmann 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> The IMPULSE special-cases did not sample the center of the of the region.
>> This caused it to sample the filters outside their range, and produce
>> assymetric filters and other errors. Fixing this required changing the
>> arguments to integral() so the correct point could be determined.
>>
>> I fixed the nice filter and the integration to directly produce normalized
>> values. Re-normalization is still needed for impulse.box or
>> impulse.triangle
>> so I did not remove it.
>>
>> Distribute fixed error over all filter samples, to remove a high-frequency
>> bit of noise in the center of some filters (lancoz at large scale value).
>>
>> box.box, which I expect will be very common as it is the proposed "good"
>> filter,
>> was made a lot faster and more accurate. This is easy as the caller
>> already
>> intersected the two boxes, so the width is the integral.
>>
>> v7: This is a merge of 4 patches and lots of new code cleanup and fixes
>>  determined by examining the gnuplot output
>>
>
> Please split this back out into separate patches that each fix one problem:
>
> - Fixing IMPULSE special cases
> - Distributing errors evenly
> - BOX.BOX speedups
>
> There are several of these changes I'm not convinced about, and they are
> hard to review as one big patch.
>

I will try to split them up. This is the unfortunate result of trying to
rebase them together, I merged because of conflicts. As you don't like the
argument renaming I might as well fix all of these at the same time.

diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
>
>> index 8b8fb82..e82871f 100644
>> --- a/pixman/pixman-filter.c
>> +++ b/pixman/pixman-filter.c
>> @@ -79,7 +79,7 @@ sinc (double x)
>>  }
>>
>>  static double
>> -lanczos (double x, int n)
>> +lanczos (double x, double n)
>>  {
>>  return sinc (x) * sinc (x * (1.0 / n));
>>  }
>>
>
> What is the point of this change? I don't think Lanczos makes sense with a
> non-integral number of lobes.
>

It removes the int->float cast, but I think this was mostly accidental.
Inline would probably remove the cast as well, and the division too.

@@ -99,7 +99,7 @@ lanczos3_kernel (double x)
>>  static double
>>  nice_kernel (double x)
>>  {
>> -return lanczos3_kernel (x * 0.75);
>> +return lanczos3_kernel (x * 0.75) * 0.75;
>>  }
>>
>
> I don't think this makes sense given that normalization is happening later
> anyway.
>

It is the only non-normalized one, and fixing this was very useful for
finding out whether bugs were in the normalization or in the filter
generators. Also it is plausable that future ones may be able to remove the
normalization, though I could not because of the box and triangle being so
non-continuous that they needed it.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v13 08/14] pixman-filter: Corrections to the integral() function

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> The IMPULSE special-cases did not sample the center of the of the region.
> This caused it to sample the filters outside their range, and produce
> assymetric filters and other errors. Fixing this required changing the
> arguments to integral() so the correct point could be determined.
>
> I fixed the nice filter and the integration to directly produce normalized
> values. Re-normalization is still needed for impulse.box or
> impulse.triangle
> so I did not remove it.
>
> Distribute fixed error over all filter samples, to remove a high-frequency
> bit of noise in the center of some filters (lancoz at large scale value).
>
> box.box, which I expect will be very common as it is the proposed "good"
> filter,
> was made a lot faster and more accurate. This is easy as the caller already
> intersected the two boxes, so the width is the integral.
>
> v7: This is a merge of 4 patches and lots of new code cleanup and fixes
>  determined by examining the gnuplot output
>

Please split this back out into separate patches that each fix one problem:

- Fixing IMPULSE special cases
- Distributing errors evenly
- BOX.BOX speedups

There are several of these changes I'm not convinced about, and they are
hard to review as one big patch.

diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c

> index 8b8fb82..e82871f 100644
> --- a/pixman/pixman-filter.c
> +++ b/pixman/pixman-filter.c
> @@ -79,7 +79,7 @@ sinc (double x)
>  }
>
>  static double
> -lanczos (double x, int n)
> +lanczos (double x, double n)
>  {
>  return sinc (x) * sinc (x * (1.0 / n));
>  }
>

What is the point of this change? I don't think Lanczos makes sense with a
non-integral number of lobes.

@@ -99,7 +99,7 @@ lanczos3_kernel (double x)
>  static double
>  nice_kernel (double x)
>  {
> -return lanczos3_kernel (x * 0.75);
> +return lanczos3_kernel (x * 0.75) * 0.75;
>  }
>

I don't think this makes sense given that normalization is happening later
anyway.


Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH v13 08/14] pixman-filter: Corrections to the integral() function

[spitzak]

From: Bill Spitzak 

The IMPULSE special-cases did not sample the center of the of the region.
This caused it to sample the filters outside their range, and produce
assymetric filters and other errors. Fixing this required changing the
arguments to integral() so the correct point could be determined.

I fixed the nice filter and the integration to directly produce normalized
values. Re-normalization is still needed for impulse.box or impulse.triangle
so I did not remove it.

Distribute fixed error over all filter samples, to remove a high-frequency
bit of noise in the center of some filters (lancoz at large scale value).

box.box, which I expect will be very common as it is the proposed "good" filter,
was made a lot faster and more accurate. This is easy as the caller already
intersected the two boxes, so the width is the integral.

v7: This is a merge of 4 patches and lots of new code cleanup and fixes
 determined by examining the gnuplot output

v9: Restored the recursion splitting at zero for linear filter

v10: Small change from here moved to previous Simpsons patch so it compiles
 Merged patch to get correct subsample positions when subsample_bits==0

v11: Whitespace fixes

Signed-off-by: Bill Spitzak 
Acked-by: Oded Gabbay 
---
 pixman/pixman-filter.c | 154 +
 1 file changed, 79 insertions(+), 75 deletions(-)

diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
index 8b8fb82..e82871f 100644
--- a/pixman/pixman-filter.c
+++ b/pixman/pixman-filter.c
@@ -79,7 +79,7 @@ sinc (double x)
 }
 
 static double
-lanczos (double x, int n)
+lanczos (double x, double n)
 {
 return sinc (x) * sinc (x * (1.0 / n));
 }
@@ -99,7 +99,7 @@ lanczos3_kernel (double x)
 static double
 nice_kernel (double x)
 {
-return lanczos3_kernel (x * 0.75);
+return lanczos3_kernel (x * 0.75) * 0.75;
 }
 
 static double
@@ -147,45 +147,51 @@ static const filter_info_t filters[] =
 { PIXMAN_KERNEL_LANCZOS3_STRETCHED, nice_kernel,  8.0 },
 };
 
-/* This function scales the @sample filter by @size, then
- * aligns @x1 in @reconstruct with @x2 in @sample and
- * and integrates the product of the kernels across @width.
+/* This function scales the @sample filter by @size, shifts it by @pos,
+ * and then integrates the product of the kernels across low..high
  *
  * This function assumes that the intervals are within
  * the kernels in question. E.g., the caller must not
  * try to integrate a linear kernel ouside of [-1:1]
  */
 static double
-integral (pixman_kernel_t reconstruct, double x1,
- pixman_kernel_t sample, double size, double x2,
- double width)
+integral (pixman_kernel_t reconstruct,
+ pixman_kernel_t sample, double size, double pos,
+ double low, double high)
 {
+if (high < low)
+{
+   return 0.0;
+}
+else if (sample == PIXMAN_KERNEL_IMPULSE)
+{
+   return filters[reconstruct].func (-pos);
+}
+else if (reconstruct == PIXMAN_KERNEL_IMPULSE)
+{
+   return filters[sample].func (-pos / size) / size;
+}
+else if (reconstruct == PIXMAN_KERNEL_BOX && sample == PIXMAN_KERNEL_BOX)
+{
+   assert (high <= low + 1.0);
+   return (high - low) / size;
+}
 /* If the integration interval crosses zero, break it into
  * two separate integrals. This ensures that filters such
  * as LINEAR that are not differentiable at 0 will still
  * integrate properly.
  */
-if (x1 < 0 && x1 + width > 0)
+else if (reconstruct == PIXMAN_KERNEL_LINEAR && low < 0 && high > 0)
 {
return
-   integral (reconstruct, x1, sample, size, x2, - x1) +
-   integral (reconstruct, 0, sample, size, x2 - x1, width + x1);
+   integral (reconstruct, sample, size, pos, low, 0) +
+   integral (reconstruct, sample, size, pos, 0, high);
 }
-else if (x2 < 0 && x2 + width > 0)
+else if (sample == PIXMAN_KERNEL_LINEAR && low < pos && high > pos)
 {
return
-   integral (reconstruct, x1, sample, size, x2, - x2) +
-   integral (reconstruct, x1 - x2, sample, size, 0, width + x2);
-}
-else if (reconstruct == PIXMAN_KERNEL_IMPULSE)
-{
-   assert (width == 0.0);
-   return filters[sample].func (x2 / size);
-}
-else if (sample == PIXMAN_KERNEL_IMPULSE)
-{
-   assert (width == 0.0);
-   return filters[reconstruct].func (x1);
+   integral (reconstruct, sample, size, pos, low, pos) +
+   integral (reconstruct, sample, size, pos, pos, high);
 }
 else
 {
@@ -197,32 +203,30 @@ integral (pixman_kernel_t reconstruct, double x1,
 * filter is 6 wide.
 */
 #define N_SEGMENTS 12
-#define SAMPLE(a1, a2) \
-   (filters[reconstruct].func ((a1)) * filters[sample].func ((a2) / size))
-   
+#define SAMPLE(a)