[Pixman] [PATCH v14 10/22] pixman-filter: Correct integration with impulse filters

[Bill Spitzak]

On Mon, Mar 21, 2016 at 7:41 PM, Søren Sandmann <soren.sandmann at gmail.com>
wrote:

>
>
> On Mon, Mar 21, 2016 at 12:29 PM, Bill Spitzak <spitzak at gmail.com> wrote:
>
>> On Sun, Mar 20, 2016 at 11:36 PM, Søren Sandmann <
>> soren.sandmann at gmail.com> wrote:
>>
>>> On Sun, Mar 6, 2016 at 8:06 PM, <spitzak at gmail.com> wrote:
>>>
>>>> From: Bill Spitzak <spitzak at gmail.com>
>>>>
>>>> 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 don't understand what is wrong and why this patch fixes it. Which
>>> region precisely did not have its center sampled? When IMPULSE filters are
>>> involved the width of the integral is 0 so there isn't really any "region"
>>> to sample.
>>>
>>> Can you give a concrete example where the previous code produced
>>> asymmetric filters? Also, what "other errors" was produced? I think these
>>> examples should be added to the commit log.
>>>
>>
>> It sampled the *other* filter (the one that is not impulse) at the left
>> edge of the region being passed, rather than at the location of the center
>> of the impulse filter. This was detected by putting asserts in the filter
>> functions to see if they were being called outside their width.
>>
>
> I tried adding such asserts and I couldn't make them trigger with the
> scale demo. It would be helpful if you could give a specific pair of
> filters and scale factor where a filter is sampled outside its width.
>
> And it really doesn't make sense to talk about the "region being passed"
> when one of the filters is IMPULSE. In that case, the width parameter is
> always 0 so there is no "region".
>

Your version relies on width==0 when either filter is impulse. This is how
it is being called right now, but I think it a very good idea to avoid
this. This patch series does not include fixes I attempted to make the
picture not vanish when impulse+impulse was selected. Those changes made
the impulse filter act like it had a width of 1 and easily triggered the
asserts.

I think my version is a lot clearer. The center of the sampling filter is
at pos, an argument to the function, and the range for the integral is
separated from the alignment and scale. I think it is obvious that this
reduces the size of both the implementation and the calling code, in
particular the sampling only needs a single argument rather than two.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pixman/attachments/20160322/543d1cce/attachment.html>