Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

On Mon, Apr 11, 2016 at 12:35 PM, Søren Sandmann 
wrote:

> On Mon, Apr 11, 2016 at 2:43 PM, Bill Spitzak  wrote:
>
>
>> I feel this can be fixed. It is already correct for subsample_bits==0.
>> Since both the filter generator and filtering code would be changed in the
>> same version, only programs that generate their own filters would actually
>> be incompatible. And as you point out the error is very tiny for large
>> numbers of subsamples, and Cairo is already using an excessively large
>> number of subsamples (likely because I was trying to remove the difference
>> between identity filters and nearest filtering, and did not realize this
>> was the underlying problem).
>>
>
> It is technically an ABI break, and cairo 1.14 did ship with a
> copied-and-pasted filter generator that assumes the current subpixel
> positioning.
>
> But yeah, maybe we can just ignore that, if we make sure a there is a
> cairo 1.14.x update that uses pixman_filter_create_separate_convolution()
> instead of the copied-and-pasted filter generator.
>
> Other than that, the fix should be straight-forward enough.
>

As I wrote that Cairo patch I can state that I think it would be perfectly
fine to make this change. The subsamples are set unnaturally high in that
patch and hide the problem. Also it is possible my filter generator is
producing centered samples so this could be an improvement, need to check.
In addition, other than the (abandoned) idea of getting good/best into
pixman, this patch series is also designed so that Cairo can use the pixman
filter generator rather than it's own.
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Søren Sandmann]

On Mon, Apr 11, 2016 at 2:43 PM, Bill Spitzak  wrote:


> I feel this can be fixed. It is already correct for subsample_bits==0.
> Since both the filter generator and filtering code would be changed in the
> same version, only programs that generate their own filters would actually
> be incompatible. And as you point out the error is very tiny for large
> numbers of subsamples, and Cairo is already using an excessively large
> number of subsamples (likely because I was trying to remove the difference
> between identity filters and nearest filtering, and did not realize this
> was the underlying problem).
>

It is technically an ABI break, and cairo 1.14 did ship with a
copied-and-pasted filter generator that assumes the current subpixel
positioning.

But yeah, maybe we can just ignore that, if we make sure a there is a cairo
1.14.x update that uses pixman_filter_create_separate_convolution() instead
of the copied-and-pasted filter generator.

Other than that, the fix should be straight-forward enough.


Søren


On Sun, Apr 10, 2016 at 10:01 PM, Søren Sandmann 
wrote:

>
> It does look like there is something really wrong. I compared and (except
>> for the subsample_bits==0 case) my version produces the same output as the
>> current git head.
>>
>> I think your intention is that there is a sample at offset=0 whether the
>> filter width is even or odd. However (except when subsample_bits==0) the
>> filter generator makes a symmetric filter for even sizes, with two equal
>> samples around the maximum center value. If a sample was at offset==0 then
>> it would be unique and larger than all the other samples.
>>
>
> The root of this confusion is probably that when subsample_bits = k, the
> subpixel positions used are:
>
> 0.5 / 2^k, 1.5 / 2^k, ..., (2^k-0.5)/2^k
>
> For example, for subsample_bits = 2:
>
> 0.125, 0.375, 0.625, 0.875
>
> and for subsample_bits = 0:
>
> 0.5
>
> That is, they are regularly spaced, but centered within the pixel. When
> there is an even number of them, this means there will not be a filter
> position at 0.5, and therefore no sample at offset 0. And the only case
> where number of subpixel locations is odd, is when subsample_bits = 0.
>
> I'm pretty sure that the existing code gets the filter generation right
> for these subpixel positions.
>
>
> [ You can argue that it would be better to use the sampling positions
>
> 0, 0.25, 0.5, 0.75
>
> for subsample_bits = 2, as Owen did here:
>
> https://lists.cairographics.org/archives/cairo/2014-March/025105.html
>
> and I agree that that would have been better. ]
>
>
> Søren
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

Okay the actual bug is that the gnuplot output is wrong for
subsample_bits==0. It apparently is correct for other values of
subsampling. I will try to get an updated version posted soon.

But I very much agree with Owen (and you?) that the current behavior is
incorrect and should be fixed exactly as stated. The current version
produces different images from fallbacks to impulse, bilinear, or
integer-sized box filters, when the filter math says the fallback should
produce an identical result. Scaling down by an integer produces a slightly
blurry and offset result, when a perfect result could be achieved at the
same speed. Increasing the subsamples moves the existing samples, thus may
increase artifacts, rather than always reducing them. All of this is not a
good situation.

I feel this can be fixed. It is already correct for subsample_bits==0.
Since both the filter generator and filtering code would be changed in the
same version, only programs that generate their own filters would actually
be incompatible. And as you point out the error is very tiny for large
numbers of subsamples, and Cairo is already using an excessively large
number of subsamples (likely because I was trying to remove the difference
between identity filters and nearest filtering, and did not realize this
was the underlying problem).

On Sun, Apr 10, 2016 at 10:01 PM, Søren Sandmann 
wrote:

>
> It does look like there is something really wrong. I compared and (except
>> for the subsample_bits==0 case) my version produces the same output as the
>> current git head.
>>
>> I think your intention is that there is a sample at offset=0 whether the
>> filter width is even or odd. However (except when subsample_bits==0) the
>> filter generator makes a symmetric filter for even sizes, with two equal
>> samples around the maximum center value. If a sample was at offset==0 then
>> it would be unique and larger than all the other samples.
>>
>
> The root of this confusion is probably that when subsample_bits = k, the
> subpixel positions used are:
>
> 0.5 / 2^k, 1.5 / 2^k, ..., (2^k-0.5)/2^k
>
> For example, for subsample_bits = 2:
>
> 0.125, 0.375, 0.625, 0.875
>
> and for subsample_bits = 0:
>
> 0.5
>
> That is, they are regularly spaced, but centered within the pixel. When
> there is an even number of them, this means there will not be a filter
> position at 0.5, and therefore no sample at offset 0. And the only case
> where number of subpixel locations is odd, is when subsample_bits = 0.
>
> I'm pretty sure that the existing code gets the filter generation right
> for these subpixel positions.
>
>
> [ You can argue that it would be better to use the sampling positions
>
> 0, 0.25, 0.5, 0.75
>
> for subsample_bits = 2, as Owen did here:
>
> https://lists.cairographics.org/archives/cairo/2014-March/025105.html
>
> and I agree that that would have been better. ]
>
>
> Søren
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Søren Sandmann]

> It does look like there is something really wrong. I compared and (except
> for the subsample_bits==0 case) my version produces the same output as the
> current git head.
>
> I think your intention is that there is a sample at offset=0 whether the
> filter width is even or odd. However (except when subsample_bits==0) the
> filter generator makes a symmetric filter for even sizes, with two equal
> samples around the maximum center value. If a sample was at offset==0 then
> it would be unique and larger than all the other samples.
>

The root of this confusion is probably that when subsample_bits = k, the
subpixel positions used are:

0.5 / 2^k, 1.5 / 2^k, ..., (2^k-0.5)/2^k

For example, for subsample_bits = 2:

0.125, 0.375, 0.625, 0.875

and for subsample_bits = 0:

0.5

That is, they are regularly spaced, but centered within the pixel. When
there is an even number of them, this means there will not be a filter
position at 0.5, and therefore no sample at offset 0. And the only case
where number of subpixel locations is odd, is when subsample_bits = 0.

I'm pretty sure that the existing code gets the filter generation right for
these subpixel positions.


[ You can argue that it would be better to use the sampling positions

0, 0.25, 0.5, 0.75

for subsample_bits = 2, as Owen did here:

https://lists.cairographics.org/archives/cairo/2014-March/025105.html

and I agree that that would have been better. ]


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

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

On 04/09/2016 01:13 PM, Søren Sandmann wrote:

On Sat, Apr 9, 2016 at 3:45 PM, Bill Spitzak > wrote:

On 04/03/2016 11:17 AM, Søren Sandmann wrote:

I don't believe this patch is correct.

As an example, consider an image with an identity transformation
and a
cubic filter (which has width 4) with subsample_bits = 0. The
current
code will compute a matrix

  [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]

Now suppose we are filtering a pixel located at x = 17.5. The
code in
bits_image_fetch_pixel_separable_convolution() will eventually
end up
convolving with the pixels at locations 15.5, 16.5, 17.5 and
18.5, which
is the correct result since cubic(-2) = 0 [1].

With your code, the matrix computed would be

  [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]

which would not be correct no matter what: With an identity
transformation, the pixel at 17.5 should be multiplied with
cubic(0).

There is a detailed explanation of these issues in the file
pixman/rounding.txt.


I have finally checked this and it is producing the correct value in
the above example. pos is not the left edge of the range to
integrate, it is the center of it, due to the changes I made to the
integrate function.


No, it really doesn't produce the correct values. If you apply the patch
below on top of your series and run demos/scale with

 subsample set to 0,
 reconstruct set to 'Cubic',
 sample set to 'Impulse'
 scale factor set to 1.0,

it prints out this:

 cubic(1.50) = -0.034722
 cubic(0.50) = 0.534722
 cubic(-0.50) = 0.534722
 cubic(-1.50) = -0.034722
 [ -0.035, 0.535, 0.535, -0.035, ]

which is not the correct result. The first value should be cubic(-2) = 0.

And if you toggle back and forth between subsamples = 0 and 1, you can
see the image shift slightly.

(The *gnuplot* graph looks correct, but that's because the gnuplot
function is also generating the wrong coordinates).


It does look like there is something really wrong. I compared and 
(except for the subsample_bits==0 case) my version produces the same 
output as the current git head.


I think your intention is that there is a sample at offset=0 whether the 
filter width is even or odd. However (except when subsample_bits==0) the 
filter generator makes a symmetric filter for even sizes, with two equal 
samples around the maximum center value. If a sample was at offset==0 
then it would be unique and larger than all the other samples.


Comparing box+box with subsample_bits==1 with bilinear, it does appear 
there is an offset and the filter is wrong. It matches exactly (in your 
version) when subsample_bits==0.


The gnuplot output is wrong because I adjusted it to make this output 
look correct.


I then further broke the only case that actually worked (when 
subsample_bits==0) with this patch, since it did not match the others.


I need to look at the actual filter sampling code and at your paper I 
guess. I think that is doing the right thing, and the generator and 
gnuplot have to be adjusted to match.




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

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Søren Sandmann]

On Sat, Apr 9, 2016 at 3:45 PM, Bill Spitzak  wrote:

> On 04/03/2016 11:17 AM, Søren Sandmann wrote:
>
>> I don't believe this patch is correct.
>>
>> As an example, consider an image with an identity transformation and a
>> cubic filter (which has width 4) with subsample_bits = 0. The current
>> code will compute a matrix
>>
>>  [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]
>>
>> Now suppose we are filtering a pixel located at x = 17.5. The code in
>> bits_image_fetch_pixel_separable_convolution() will eventually end up
>> convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which
>> is the correct result since cubic(-2) = 0 [1].
>>
>> With your code, the matrix computed would be
>>
>>  [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]
>>
>> which would not be correct no matter what: With an identity
>> transformation, the pixel at 17.5 should be multiplied with cubic(0).
>>
>> There is a detailed explanation of these issues in the file
>> pixman/rounding.txt.
>>
>
> I have finally checked this and it is producing the correct value in the
> above example. pos is not the left edge of the range to integrate, it is
> the center of it, due to the changes I made to the integrate function.
>

No, it really doesn't produce the correct values. If you apply the patch
below on top of your series and run demos/scale with

subsample set to 0,
reconstruct set to 'Cubic',
sample set to 'Impulse'
scale factor set to 1.0,

it prints out this:

cubic(1.50) = -0.034722
cubic(0.50) = 0.534722
cubic(-0.50) = 0.534722
cubic(-1.50) = -0.034722
[ -0.035, 0.535, 0.535, -0.035, ]

which is not the correct result. The first value should be cubic(-2) = 0.

And if you toggle back and forth between subsamples = 0 and 1, you can see
the image shift slightly.

(The *gnuplot* graph looks correct, but that's because the gnuplot function
is also generating the wrong coordinates).


Søren



diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
index 2bf289a..52b7834 100644
--- a/pixman/pixman-filter.c
+++ b/pixman/pixman-filter.c
@@ -134,7 +134,9 @@ cubic_kernel (double x)
  * (0.0, 0.5) would give us the Catmull-Rom spline,
  * but that one seems to be indistinguishable from Lanczos2.
  */
-return general_cubic (x, 1/3.0, 1/3.0);
+double v = general_cubic (x, 1/3.0, 1/3.0);
+printf ("cubic(%f) = %f\n", x, v);
+return v;
 }

 static const filter_info_t filters[] =
@@ -297,6 +299,11 @@ create_1d_filter (int  width,
}
}

+   printf ("[ ");
+   for (x = 0; x < width; ++x)
+   printf ("%.3f, ", pixman_fixed_to_double (p[x]));
+   printf ("]\n");
+
p += width;
 }
 }
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

On 04/03/2016 11:17 AM, Søren Sandmann wrote:

I don't believe this patch is correct.

As an example, consider an image with an identity transformation and a
cubic filter (which has width 4) with subsample_bits = 0. The current
code will compute a matrix

 [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]

Now suppose we are filtering a pixel located at x = 17.5. The code in
bits_image_fetch_pixel_separable_convolution() will eventually end up
convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which
is the correct result since cubic(-2) = 0 [1].

With your code, the matrix computed would be

 [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]

which would not be correct no matter what: With an identity
transformation, the pixel at 17.5 should be multiplied with cubic(0).

There is a detailed explanation of these issues in the file
pixman/rounding.txt.


I have finally checked this and it is producing the correct value in the 
above example. pos is not the left edge of the range to integrate, it is 
the center of it, due to the changes I made to the integrate function.


However the code is incorrect and only works because n_phases&1 is true 
only when subsample_bits == 0. The intention was to treat all odd widths 
the same.


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

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

I believe there may have been a rebasing error. Will look into it.

I certainly intended this to change behavior when the filter size is odd,
not when the number of subsamples is odd. Not sure if this is truly rebase
screwing up, or I just mistyped an attempt to fix a rebase error.


On Mon, Apr 4, 2016 at 12:21 PM, Søren Sandmann 
wrote:

> No, it changes behavior when the number of *phases* is odd, which it is in
> the example I gave.
>
>
> Søren
>
> On Mon, Apr 4, 2016 at 2:51 PM, Bill Spitzak  wrote:
>
>> That's why this only changes the behavior when the number of samples is
>> odd.
>>
>>
>> On Sun, Apr 3, 2016 at 11:17 AM, Søren Sandmann > > wrote:
>>
>>> I don't believe this patch is correct.
>>>
>>> As an example, consider an image with an identity transformation and a
>>> cubic filter (which has width 4) with subsample_bits = 0. The current code
>>> will compute a matrix
>>>
>>> [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]
>>>
>>> Now suppose we are filtering a pixel located at x = 17.5. The code in
>>> bits_image_fetch_pixel_separable_convolution() will eventually end up
>>> convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which is
>>> the correct result since cubic(-2) = 0 [1].
>>>
>>> With your code, the matrix computed would be
>>>
>>> [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]
>>>
>>> which would not be correct no matter what: With an identity
>>> transformation, the pixel at 17.5 should be multiplied with cubic(0).
>>>
>>> There is a detailed explanation of these issues in the file
>>> pixman/rounding.txt.
>>>
>>>
>>> Søren
>>>
>>> [1] You might consider optimizing this case away and generate a matrix
>>> with width 3, but since this would only work with subsample_bits=0, it's
>>> not worth the special-casing.
>>>
>>>
>>> On Sun, Mar 6, 2016 at 8:06 PM,  wrote:
>>>
 From: Bill Spitzak 

 The position of only one subsample was wrong as ceil() was done on an
 integer.
 Use a different function for all odd numbers of subsamples that gets
 this right.

 Signed-off-by: Bill Spitzak 
 ---
  pixman/pixman-filter.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
 index ac89dda..f9ad45f 100644
 --- a/pixman/pixman-filter.c
 +++ b/pixman/pixman-filter.c
 @@ -252,7 +252,10 @@ create_1d_filter (int  width,
  * and sample positions.
  */

 -   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
 +   if (n_phases & 1)
 +   pos = frac - width / 2.0;
 +   else
 +   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;

 total = 0;
 for (x = 0; x < width; ++x, ++pos)
 --
 1.9.1

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

>>>
>>>
>>
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Søren Sandmann]

No, it changes behavior when the number of *phases* is odd, which it is in
the example I gave.


Søren

On Mon, Apr 4, 2016 at 2:51 PM, Bill Spitzak  wrote:

> That's why this only changes the behavior when the number of samples is
> odd.
>
>
> On Sun, Apr 3, 2016 at 11:17 AM, Søren Sandmann 
> wrote:
>
>> I don't believe this patch is correct.
>>
>> As an example, consider an image with an identity transformation and a
>> cubic filter (which has width 4) with subsample_bits = 0. The current code
>> will compute a matrix
>>
>> [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]
>>
>> Now suppose we are filtering a pixel located at x = 17.5. The code in
>> bits_image_fetch_pixel_separable_convolution() will eventually end up
>> convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which is
>> the correct result since cubic(-2) = 0 [1].
>>
>> With your code, the matrix computed would be
>>
>> [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]
>>
>> which would not be correct no matter what: With an identity
>> transformation, the pixel at 17.5 should be multiplied with cubic(0).
>>
>> There is a detailed explanation of these issues in the file
>> pixman/rounding.txt.
>>
>>
>> Søren
>>
>> [1] You might consider optimizing this case away and generate a matrix
>> with width 3, but since this would only work with subsample_bits=0, it's
>> not worth the special-casing.
>>
>>
>> On Sun, Mar 6, 2016 at 8:06 PM,  wrote:
>>
>>> From: Bill Spitzak 
>>>
>>> The position of only one subsample was wrong as ceil() was done on an
>>> integer.
>>> Use a different function for all odd numbers of subsamples that gets
>>> this right.
>>>
>>> Signed-off-by: Bill Spitzak 
>>> ---
>>>  pixman/pixman-filter.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
>>> index ac89dda..f9ad45f 100644
>>> --- a/pixman/pixman-filter.c
>>> +++ b/pixman/pixman-filter.c
>>> @@ -252,7 +252,10 @@ create_1d_filter (int  width,
>>>  * and sample positions.
>>>  */
>>>
>>> -   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
>>> +   if (n_phases & 1)
>>> +   pos = frac - width / 2.0;
>>> +   else
>>> +   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
>>>
>>> total = 0;
>>> for (x = 0; x < width; ++x, ++pos)
>>> --
>>> 1.9.1
>>>
>>> ___
>>> Pixman mailing list
>>> Pixman@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/pixman
>>>
>>
>>
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Bill Spitzak]

That's why this only changes the behavior when the number of samples is odd.


On Sun, Apr 3, 2016 at 11:17 AM, Søren Sandmann 
wrote:

> I don't believe this patch is correct.
>
> As an example, consider an image with an identity transformation and a
> cubic filter (which has width 4) with subsample_bits = 0. The current code
> will compute a matrix
>
> [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]
>
> Now suppose we are filtering a pixel located at x = 17.5. The code in
> bits_image_fetch_pixel_separable_convolution() will eventually end up
> convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which is
> the correct result since cubic(-2) = 0 [1].
>
> With your code, the matrix computed would be
>
> [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]
>
> which would not be correct no matter what: With an identity
> transformation, the pixel at 17.5 should be multiplied with cubic(0).
>
> There is a detailed explanation of these issues in the file
> pixman/rounding.txt.
>
>
> Søren
>
> [1] You might consider optimizing this case away and generate a matrix
> with width 3, but since this would only work with subsample_bits=0, it's
> not worth the special-casing.
>
>
> On Sun, Mar 6, 2016 at 8:06 PM,  wrote:
>
>> From: Bill Spitzak 
>>
>> The position of only one subsample was wrong as ceil() was done on an
>> integer.
>> Use a different function for all odd numbers of subsamples that gets this
>> right.
>>
>> Signed-off-by: Bill Spitzak 
>> ---
>>  pixman/pixman-filter.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
>> index ac89dda..f9ad45f 100644
>> --- a/pixman/pixman-filter.c
>> +++ b/pixman/pixman-filter.c
>> @@ -252,7 +252,10 @@ create_1d_filter (int  width,
>>  * and sample positions.
>>  */
>>
>> -   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
>> +   if (n_phases & 1)
>> +   pos = frac - width / 2.0;
>> +   else
>> +   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
>>
>> total = 0;
>> for (x = 0; x < width; ++x, ++pos)
>> --
>> 1.9.1
>>
>> ___
>> Pixman mailing list
>> Pixman@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/pixman
>>
>
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[Søren Sandmann]

I don't believe this patch is correct.

As an example, consider an image with an identity transformation and a
cubic filter (which has width 4) with subsample_bits = 0. The current code
will compute a matrix

[ cubic(-2), cubic(-1), cubic(0), cubic(1) ]

Now suppose we are filtering a pixel located at x = 17.5. The code in
bits_image_fetch_pixel_separable_convolution() will eventually end up
convolving with the pixels at locations 15.5, 16.5, 17.5 and 18.5, which is
the correct result since cubic(-2) = 0 [1].

With your code, the matrix computed would be

[ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]

which would not be correct no matter what: With an identity transformation,
the pixel at 17.5 should be multiplied with cubic(0).

There is a detailed explanation of these issues in the file
pixman/rounding.txt.


Søren

[1] You might consider optimizing this case away and generate a matrix with
width 3, but since this would only work with subsample_bits=0, it's not
worth the special-casing.


On Sun, Mar 6, 2016 at 8:06 PM,  wrote:

> From: Bill Spitzak 
>
> The position of only one subsample was wrong as ceil() was done on an
> integer.
> Use a different function for all odd numbers of subsamples that gets this
> right.
>
> Signed-off-by: Bill Spitzak 
> ---
>  pixman/pixman-filter.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
> index ac89dda..f9ad45f 100644
> --- a/pixman/pixman-filter.c
> +++ b/pixman/pixman-filter.c
> @@ -252,7 +252,10 @@ create_1d_filter (int  width,
>  * and sample positions.
>  */
>
> -   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
> +   if (n_phases & 1)
> +   pos = frac - width / 2.0;
> +   else
> +   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
>
> total = 0;
> for (x = 0; x < width; ++x, ++pos)
> --
> 1.9.1
>
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pixman
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0

[spitzak]

From: Bill Spitzak 

The position of only one subsample was wrong as ceil() was done on an integer.
Use a different function for all odd numbers of subsamples that gets this right.

Signed-off-by: Bill Spitzak 
---
 pixman/pixman-filter.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c
index ac89dda..f9ad45f 100644
--- a/pixman/pixman-filter.c
+++ b/pixman/pixman-filter.c
@@ -252,7 +252,10 @@ create_1d_filter (int  width,
 * and sample positions.
 */
 
-   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
+   if (n_phases & 1)
+   pos = frac - width / 2.0;
+   else
+   pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;
 
total = 0;
for (x = 0; x < width; ++x, ++pos)
-- 
1.9.1

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