<div dir="ltr"><div>No, it changes behavior when the number of *phases* is odd, which it is in the example I gave.<br><br><br></div>Søren<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 4, 2016 at 2:51 PM, Bill Spitzak <span dir="ltr"><<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">That's why this only changes the behavior when the number of samples is odd.<div><br></div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Apr 3, 2016 at 11:17 AM, Søren Sandmann <span dir="ltr"><<a href="mailto:soren.sandmann@gmail.com" target="_blank">soren.sandmann@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>I don't believe this patch is correct.<br><br>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<br><br></div> [ cubic(-2), cubic(-1), cubic(0), cubic(1) ]<br><br></div><div>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].<br><br>With your code, the matrix computed would be<br><br></div><div> [ cubic(-1.5), cubic(-0.5), cubic(0.5), cubic(1) ]<br><br></div><div>which would not be correct no matter what: With an identity transformation, the pixel at 17.5 should be multiplied with cubic(0).<br><br>There is a detailed explanation of these issues in the file pixman/rounding.txt.<br><br></div><div><br>Søren<br></div><div><br>[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.<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Sun, Mar 6, 2016 at 8:06 PM, <span dir="ltr"><<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>From: Bill Spitzak <<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>><br> <br> The position of only one subsample was wrong as ceil() was done on an integer.<br> Use a different function for all odd numbers of subsamples that gets this right.<br> <br> Signed-off-by: Bill Spitzak <<a href="mailto:spitzak@gmail.com" target="_blank">spitzak@gmail.com</a>><br> ---<br> pixman/pixman-filter.c | 5 ++++-<br> 1 file changed, 4 insertions(+), 1 deletion(-)<br> <br> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c<br> index ac89dda..f9ad45f 100644<br> --- a/pixman/pixman-filter.c<br> +++ b/pixman/pixman-filter.c<br> @@ -252,7 +252,10 @@ create_1d_filter (int width,<br> * and sample positions.<br> */<br> <br> - pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;<br> + if (n_phases & 1)<br> + pos = frac - width / 2.0;<br> + else<br> + pos = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac;<br> <br> total = 0;<br> for (x = 0; x < width; ++x, ++pos)<br> </div></div><span><font color="#888888"><span><font color="#888888">--<br> 1.9.1<br> <br> _______________________________________________<br> Pixman mailing list<br> <a href="mailto:Pixman@lists.freedesktop.org" target="_blank">Pixman@lists.freedesktop.org</a><br> <a href="https://lists.freedesktop.org/mailman/listinfo/pixman" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/pixman</a><br> </font></span></font></span></blockquote></div><br></div> </blockquote></div><br></div> </div></div></blockquote></div><br></div>