On Wed, 02 Sep 2015 20:34:56 +0100 "Ben Avison" <bavi...@riscosopen.org> wrote:
> On Wed, 02 Sep 2015 14:03:01 +0100, Pekka Paalanen <ppaala...@gmail.com> > wrote: > > I am still a tiny bit struggling with why 31 and not 32, but it does > > add up. > > Maybe the reasoning in the comments in random_scale_factor() make more > sense? There, it only talks about the numbers as integers, avoiding the > additional confusion caused by MAX_INC being 0.32 fixed-point and the > return value being 16.16 fixed-point. > > If you still want to think of it as real numbers, then: > > * random_scale_factor() generates a scale factor between > 2^(-LOG2_MAX_FACTOR-0.5) and 2^(LOG2_MAX_FACTOR+0.5) > * INV_SQRT_2_0POINT32_FIXED represents 2^-0.5 > * to get from 2^-0.5 to 2^(LOG2_MAX_FACTOR+0.5) we must multiply by > 2^(LOG2_MAX_FACTOR+1) > * but to convert from 0.32 to 16.16 fixed point, we multiply by 2^(-32+16) > * so the total factor is 2^(LOG2_MAX_FACTOR+1-32+16) > * but this is < 1, so is actually a shift right by > -LOG2_MAX_FACTOR-1+32-16, which can trivially be rearranged to match > the macro definition. Yes. My point is, you spell out the -16 in the code, but not the 32 - 1. Anyway, it doesn't matter, it is correct. > >> +static void > >> +check_transform (pixman_image_t *dst_img, > >> + pixman_image_t *src_img, > >> + pixman_transform_t *transform, > >> + pixman_bool_t bilinear) > >> +{ > [...] > >> + if (bilinear) > >> + { > >> + assert (v1.vector[0] >= pixman_fixed_1 / 2); > >> + assert (v1.vector[1] >= pixman_fixed_1 / 2); > >> + assert (v2.vector[0] <= pixman_int_to_fixed (src_img->bits.width) > >> - > >> + pixman_fixed_1 / 2); > >> + assert (v2.vector[1] <= pixman_int_to_fixed > >> (src_img->bits.height) - > >> + pixman_fixed_1 / 2); > > > > Since we had that discussion about bilinear filtering sampling pixels at > > n=0,1 instead of n=-1,0, should the last two asserts not have < instead > > of <=? > > No. calc_translate() specifically ensures that (depending upon low_align) > either the lower or upper coordinate exactly corresponds to the lowest or > highest value that contains no non-zero-weighted contribution from any > pixel beyond the source image (and the image sizes are chosen so that the > opposite coordinate should always fall within the source image too, at > the largest possible increment). These are in fact the very cases that > are most likely to trigger a fast path or fetcher to perform an out-of > bounds access, so they were deliberately included. Ah, yes, now I understand. This is *not* for protecting against out-of-bounds access, but this is allowing for the extreme cases where a cover path is still *theoretically* possible. (Ignoring what the COVER flags actually mean in Pixman implementation.) > > I mean, wouldn't sampling for x=width-0.5 cause reads on pixels > > n=width-1,width, where the latter would be out of bounds (even if > > weight zero)? > > My recent 7-patch series deals with precisely this case: > http://lists.freedesktop.org/archives/pixman/2015-August/003878.html Right! Now I think I'm getting the hang of this. :-) > Before this patch series, the conditions under which the > FAST_PATH_SAMPLES_COVER_CLIP_BILINEAR flag is set mean that this case > never gets passed to a "cover" fast path, even though such fast paths can > actually handle them by at least these three methods: > > * check the weight and don't load the upper sample if it's zero (it turns > out this can be achieved for free with ARM fetchers) > * invert the increments so that when x=width-0.5 it reads samples > n=width-2,width-1 and applies a weight of 0 to the former (I used this > for pixman-fast-path.c) > * detect the case in the C binding function and handle it separately (I > used this for the existing armv7/mips/sse2 fast paths) Now I understand that what you want to do with the mentioned patch series is to change the meaning of COVER_CLIP_BILINEAR flag, not only to remove the useless fuzz margins. Thanks, pq
pgpc9Tx2IeJhu.pgp
Description: OpenPGP digital signature
_______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman