[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 <soren.sandmann at gmail.com>
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 <spitzak at gmail.com> 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 <soren.sandmann at gmail.com
>> > 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, <spitzak at gmail.com> wrote:
>>>
>>>> From: Bill Spitzak <spitzak at gmail.com>
>>>>
>>>> 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 <spitzak at gmail.com>
>>>> ---
>>>>  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 at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/pixman
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pixman/attachments/20160404/a8ac840a/attachment.html>