<div dir="ltr">Seems correct and gets more cases than my code did (I was assuming nobody would use bilinear for scales less than 1/2 so I did not check for it). Minor comments:<div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 14, 2016 at 11:19 PM, Søren Sandmann Pedersen <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">Generalize and simplify the code that reduces BILINEAR to NEAREST so<br>
that all the reduction happens for all affine transformations where<br>
t00..t22 are integers and (t00 + t01) and (t10 + t11) are both<br>
odd. This is a sufficient condition for the resulting transformed<br>
coordinates being exactly at the center of a pixel so that BILINEAR<br>
becomes identical to NEAREST.<br></blockquote><div><br></div><div>Actually the last row (t20, t21, t22) has to be 0,0,1 (or 0,0,w where you then make a new matrix that divides everything by w and then it is 0,0,1). This is already tested for by the test for AFFINE, but this comment is a bit incorrect.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Signed-off-by: Søren Sandmann <<a href="mailto:soren.sandmann@gmail.com">soren.sandmann@gmail.com</a>><br>
---<br>
pixman/pixman-image.c | 69 ++++++++++++++++++++++++++++++---------------------<br>
1 file changed, 41 insertions(+), 28 deletions(-)<br>
<br>
diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c<br>
index 1ff1a49..cff8a30 100644<br>
--- a/pixman/pixman-image.c<br>
+++ b/pixman/pixman-image.c<br>
@@ -335,37 +335,50 @@ compute_image_info (pixman_image_t *image)<br>
{<br>
flags |= FAST_PATH_NEAREST_FILTER;<br>
}<br>
- else if (<br>
- /* affine and integer translation components in matrix ... */<br>
- ((flags & FAST_PATH_AFFINE_TRANSFORM) &&<br>
- !pixman_fixed_frac (image->common.transform->matrix[0][2] |<br>
- image->common.transform->matrix[1][2])) &&<br>
- (<br>
- /* ... combined with a simple rotation */<br>
- (flags & (FAST_PATH_ROTATE_90_TRANSFORM |<br>
- FAST_PATH_ROTATE_180_TRANSFORM |<br>
- FAST_PATH_ROTATE_270_TRANSFORM)) ||<br>
- /* ... or combined with a simple non-rotated translation */<br>
- (image->common.transform->matrix[0][0] == pixman_fixed_1 &&<br>
- image->common.transform->matrix[1][1] == pixman_fixed_1 &&<br>
- image->common.transform->matrix[0][1] == 0 &&<br>
- image->common.transform->matrix[1][0] == 0)<br>
- )<br>
- )<br>
+ else if (flags & FAST_PATH_AFFINE_TRANSFORM)<br>
{<br>
- /* FIXME: there are some affine-test failures, showing that<br>
- * handling of BILINEAR and NEAREST filter is not quite<br>
- * equivalent when getting close to 32K for the translation<br>
- * components of the matrix. That's likely some bug, but for<br>
- * now just skip BILINEAR->NEAREST optimization in this case.<br>
+ /* Suppose the transform is<br>
+ *<br>
+ * [ t00, t01, t02 ]<br>
+ * [ t10, t11, t12 ]<br>
+ * [ 120, t21, t22 ]<br></blockquote><div><br></div><div>typo 120->t20. I would change the last row to just read [0,0,1] as you are assuming this and not using these values in the following equation.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ *<br>
+ * and the destination coordinates are (n + 0.5, m + 0.5). Then<br>
+ * the transformed x coordinate is:<br>
+ *<br>
+ * tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02<br>
+ * = t00 * n + t01 * m + t02 + (t00 + t01) * 0.5<br></blockquote><div><br></div><div>I would use u,v rather than tx,ty for the source coordinates. More complex analysis of transforms gets very hard to read unless the variables are kept to one letter.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ * which implies that if t00, t01 and t02 are all integers<br>
+ * and (t00 + t01) is odd, then tx will be an integer plus 0.5,<br>
+ * which means a BILINEAR filter will reduce to NEAREST. The same<br>
+ * applies in the y direction<br>
*/<br>
- pixman_fixed_t magic_limit = pixman_int_to_fixed (30000);<br>
- if (image->common.transform->matrix[0][2] <= magic_limit &&<br>
- image->common.transform->matrix[1][2] <= magic_limit &&<br>
- image->common.transform->matrix[0][2] >= -magic_limit &&<br>
- image->common.transform->matrix[1][2] >= -magic_limit)<br>
+ pixman_fixed_t (*t)[3] = image->common.transform->matrix;<br>
+<br>
+ if (pixman_fixed_frac (t[0][0]) == 0 &&<br>
+ pixman_fixed_frac (t[0][1]) == 0 &&<br>
+ pixman_fixed_frac (t[0][2]) == 0 &&<br>
+ ((t[0][0] + t[0][1]) & pixman_fixed_1) == pixman_fixed_1 &&<br>
+ pixman_fixed_frac (t[1][0]) == 0 &&<br>
+ pixman_fixed_frac (t[1][1]) == 0 &&<br>
+ pixman_fixed_frac (t[1][2]) == 0 &&<br>
+ ((t[1][0] + t[1][1]) & pixman_fixed_1) == pixman_fixed_1)<br></blockquote><div><br></div><div>You can or all the numbers together to test they are all integers in one step.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
{<br>
- flags |= FAST_PATH_NEAREST_FILTER;<br>
+ /* FIXME: there are some affine-test failures, showing that<br>
+ * handling of BILINEAR and NEAREST filter is not quite<br>
+ * equivalent when getting close to 32K for the translation<br>
+ * components of the matrix. That's likely some bug, but for<br>
+ * now just skip BILINEAR->NEAREST optimization in this case.<br>
+ */<br>
+ pixman_fixed_t magic_limit = pixman_int_to_fixed (30000);<br>
+ if (image->common.transform->matrix[0][2] <= magic_limit &&<br>
+ image->common.transform->matrix[1][2] <= magic_limit &&<br>
+ image->common.transform->matrix[0][2] >= -magic_limit &&<br>
+ image->common.transform->matrix[1][2] >= -magic_limit)<br>
+ {<br>
+ flags |= FAST_PATH_NEAREST_FILTER;<br>
+ }<br></blockquote><div><br></div><div>Can this be tested to see if the bug still exists?</div><div><br></div><div>The comment should be reworded to point out that the bug is in the BILINEAR code, not the NEAREST code, but this patch is needed to not have the NEAREST case "fix" the ones that are broken and thus make the fast path output different.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
}<br>
}<br>
break;<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.7.11.7<br>
<br>
_______________________________________________<br>
Pixman mailing list<br>
<a href="mailto:Pixman@lists.freedesktop.org">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></blockquote></div><br></div></div>