Re: [Pixman] Dithering patches, v2
Is the "blue noise" version actually slower than the Bayer? Seems to be doing a bit less calculation but it reading the array of weights. The removal of the need for the pattern offset seems like a win. On Mon, Apr 22, 2019 at 9:27 AM Matt Turner wrote: > On Fri, Apr 19, 2019 at 4:52 PM Bryce Harrington > wrote: > > Inkscape would love to see Basile's dithering patches included. Our > > testing shows that they make a huge quality difference for our users; > > this solves a critical need. > > > > Mc and I have done some preliminary investigation into how to plumb this > > into Cairo, and would love to hear your review of Basile's approach to > > the problem. > > I don't feel like I'm experienced enough with that side of pixman to > offer meaningful comments. I've Cc'd Søren in the hopes that he > remains interested enough in the project to review the patches that > Basile says implement the approach Søren described. > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Dithering patches, v2
How difficult is it to just make the gradients use dithering, so they produce 8-bit (or whatever) results directly but with the dithering pattern in them? What other operations would need dithering (a large blur comes to mind, also zooming way in on an image in bilinear)? I think the blue noise large pattern is a great idea. Could have used that years ago in Nuke (it uses error diffusion but people always complained that the pattern did not reproduce when tiny changes were made to the image). On Tue, Apr 9, 2019 at 3:01 PM Basile Clement wrote: > I forgot to mention two things: > > - Enabling dithering makes processing much slower, since it forces the > wide pipeline to be used (and some optimisations are disabled). This > could be solved through the use of fast paths where needed (e.g. when > blitting images from an equal or lower bits-per-pixels format, or with > fused gradient+dithering paths). In any case, the design of the > implementation allows setting the `dither` property of a destination > image in between calls to `pixman_image_composite` where dithering is > required, allowing users to pay the cost of the slower wide pipeline > only for those calls where dithering is actually required. > > - The dithering is not gamma corrected, which doesn't matter much for > 8bpp formats -- but makes a noticeable difference for lower bpp formats, > producing images which are way too luminous (this is particularily > visible in the dithering demos when using 2bpp and 1bpp formats). Since > the main use case for now is a8r8g8b8 gradient export in Inkscape (and > since the series is starting to be a bit large), I did not include gamma > correction in this series. If needed, it can be added in subsequent > patches, and should probably be configurable since the proper curve to > use depend on the characteristic of the target device, which pixman may > not know about. > > - Basile > > On 4/9/19 11:29 PM, basile-pix...@clement.pm wrote: > > I am resubmitting for review and inclusion my series of patches on > > dithering from October, which I ended up not having the time to finish > > then. There are only a couple changes to make the patches compatible > > with the recently added floating point formats, and I have also added a > > dithering matrix based on blue noise which provides more pleasant > > results than the Bayer matrix from the first series. > > > > Maarten, I would appreciate if you can take a second look (although as > > noted above not much has changed since your first review -- I don't > > think you had comments on the dithering part?). > > > > - Basile > > > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/pixman > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Is circle rendering possible?
If you are rendering a solid color (so the result is either that color or transparent black) then you can just fill the entire RGB with that color to turn the premulitplied output into unpremultiplied. However I am unsure what your precision problem is. You would have to composite several dozen mostly-transparent pixels for it to make any visible difference. On Wed, Feb 6, 2019 at 2:28 AM Indi Kernick wrote: > I thought pixman allowed you to choose between premultiplied and not > premultiplied but I checked and apparently not. I think I'm getting my > libraries mixed up. The problem I have with premultiplied alpha is that you > lose precision when you have to un-premultiply. That's something I need in > my application. I want to get the color of a pixel without losing precision > with semi-transparent pixels. > > So if I don't want to lose precision, I'll have to store images in regular > alpha and then multiply the alpha every time I want to composite. That's > kind of annoying. > > To render a polygon, I could divide it into triangles and render those > with pixman. > > Thanks for your help. I think I'll explore other libraries. > > > On Wed, 6 Feb 2019 at 20:24 Pekka Paalanen wrote: > >> On Wed, 6 Feb 2019 19:25:48 +1030 >> Indi Kernick wrote: >> >> > The only problem I can find with Cairo is that it uses premultiplied >> alpha. >> >> Pixman uses premultiplied alpha. Also Wayland uses premultiplied alpha >> FWIW. I think you will find it quite common in general. >> >> > That's so annoying! Also, I'm not sure if the Cairo renderer is GPU or >> CPU >> > based. >> >> You choose through the API which one you use. Use the image API, and >> you get CPU rendering. >> >> > I'm using Qt to render the images. Qt can render surfaces so I can use >> > pixman to manipulate a surface and then Qt to upload it to the GPU when >> > it's time to render. Cairo can do the same thing but then I have to >> worry >> > about premultiplied alpha. I could use pixman to do the compositing and >> > Cairo to do other stuff but that's just a mess. >> > >> > I'm guessing that using a GPU backend for Cairo and then making it play >> > nice with the Qt layout engine will be a nightmare. If I have to go back >> > and forth between CPU and GPU, it might actually be faster to use CPU >> > rendering. The only thing I'm worried about is compositing. Compositing >> is >> > much faster on the GPU (I presume) but if I use Cairo, I'll have to >> fiddle >> > around with the alpha channel. That means a round trip between CPU and >> GPU >> > memory. >> >> You don't need to fiddle with alpha. Just write your fragment shaders >> and blending state to expect it. >> >> The point of using premultiplied alpha is to pre-compute values you >> would be computing in any case during blending. >> >> > I only really need non-antialiased circle rendering (antialiasing is a >> > nice-to-have). I can figure out to do that myself (Bresenham comes to >> > mind). I think I'll use pixman and do everything on the CPU. >> > >> > Another question: can pixman render a filled polygon without >> antialiasing? >> > If not, I might have to do that myself too. >> >> That I don't know. I have never used Pixman beyond >> pixman_image_composite32(). I am guessing the trapezoids or triangles >> API could be used, but there does not seem to be anything about polygons >> per se. You can see the header yourself, that is all the documentation >> there is, which is practically none. Cairo at least has documentation. >> >> I believe there are also other CPU rendering libraries, but I wouldn't >> know to point to them. >> >> >> Thanks, >> pq >> > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] pixman: Add support for argb/xrgb float formats, v5.
I don't like the fact that the design which allows the rgba to be different sizes cannot make them vary between float and decimal. The main reason is so that an integer for Alpha can be supported. I think maybe there should be a bit that changes the interpretation of the bit widths to a 16-valued type enumeration. For n-bit integers use n-1 as now, but remove the unusual ones and replace them with floating point and signed types. Use a lookup table to get type + width. I really don't think there are more than 16 of them: 8, 16, 32, 64 bit unsigned 8, 16, 32, 64 bit float this set leaves 8 unused values, which maybe could be allocated in the future for signed types or 128-bit types. You could also provide a few more unsigned widths (1,2,4,10,12?) ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 1/2] pixman: Add support for argb/xrgb float formats, v3.
I think an indicator for float can double as indicating the size is multiplied by 8. All float formats I am familiar with take a mulitple of 8 bits, including an 8-bit format that is sometimes used in CG. On Mon, Oct 29, 2018 at 2:18 AM Maarten Lankhorst < maarten.lankho...@linux.intel.com> wrote: > Op 29-10-18 om 09:16 schreef Siarhei Siamashka: > > On Wed, 03 Oct 2018 10:11:46 +0100 > > Chris Wilson wrote: > > > >> Quoting Maarten Lankhorst (2018-08-01 13:41:33) > >>> diff --git a/pixman/pixman.h b/pixman/pixman.h > >>> index 509ba5e534a8..c9bf4faa80d4 100644 > >>> --- a/pixman/pixman.h > >>> +++ b/pixman/pixman.h > >>> @@ -647,19 +647,24 @@ struct pixman_indexed > >>> * sample implementation allows only packed RGB and GBR > >>> * representations for data to simplify software rendering, > >>> */ > >>> -#define PIXMAN_FORMAT(bpp,type,a,r,g,b)(((bpp) << 24) | \ > >>> +#define PIXMAN_FORMAT_PACKC(val) ((val) <= 10 ? (val) : \ > >>> +(val) == 32 ? 11 : 0) > >>> +#define PIXMAN_FORMAT_UNPACKC(val) ((val) <= 10 ? (val) : \ > >>> + (val) == 11 ? 32 : 0) > >> We have 4bits, why not reserve 0xf for float32? Certainly you > >> want space for > >> > >> #define PIXMAN_FORMAT_PACKED_FLOAT16 0xb > >> #define PIXMAN_FORMAT_PACKED_FLOAT32 0xc > >> #define PIXMAN_FORMAT_PACKED_FLOAT64 0xd > >> > >> I just wonder if we may have 12bpc one day as well, so leaving 0xc clear > >> has some some appeal. > > We could probably also try to do something like this: > > > > /* > > * While the protocol is generous in format support, the > > * sample implementation allows only packed RGB and GBR > > * representations for data to simplify software rendering, > > * > > * Bit 23 selects the granularity of channel color depth > > * 0: 1-bit granularity (allows up to 15 bits per channel) > > * 1: 1-byte granularity (allows up to 120 bits per channel) > > */ > > #define PIXMAN_FORMAT(bpp,type,a,r,g,b) \ > > (((bpp) << 24) | \ > > ((type) << 16) | \ > > (((a) < 16) ? ((a) << 12) : (((a / 8) << 12) | (1 << 23))) | \ > > (((r) < 16) ? ((r) << 8) : (((r / 8) << 8) | (1 << 23))) | \ > > (((g) < 16) ? ((g) << 4) : (((g / 8) << 4) | (1 << 23))) | \ > > (((b) < 16) ? ((b)) : (((b / 8)) | (1 << 23 > > > > /* 0 for 1-bit granularity and 3 for 1-byte granularity */ > > #define PIXMAN_FORMAT_CHANDEPTH_SHIFT(f) \ > > f) >> 23) & 1) | f) >> 23) & 1) << 1)) > I would use 2 bits then, 6 is still plenty for the rest of the type. > > Perhaps make a separate PIXMAN_FORMAT_TYPE_RGBA_FLOAT and > PIXMAN_FORMAT_TYPE_RGB_FLOAT? > > > #define PIXMAN_FORMAT_A(f) \ > > f) >> 12) & 0x0f) << PIXMAN_FORMAT_CHANDEPTH_SHIFT(f)) > > #define PIXMAN_FORMAT_R(f) \ > > f) >> 8) & 0x0f) << PIXMAN_FORMAT_CHANDEPTH_SHIFT(f)) > > #define PIXMAN_FORMAT_G(f) \ > > f) >> 4) & 0x0f) << PIXMAN_FORMAT_CHANDEPTH_SHIFT(f)) > > #define PIXMAN_FORMAT_B(f) \ > > f) ) & 0x0f) << PIXMAN_FORMAT_CHANDEPTH_SHIFT(f)) > > > > #define PIXMAN_FORMAT_BPP(f) (((uint32_t)(f)) >> 24) > > #define PIXMAN_FORMAT_TYPE(f) (((f) >> 16) & 0x7f) > > #define PIXMAN_FORMAT_RGB(f) (((f) ) & 0xfff) > > #define PIXMAN_FORMAT_VIS(f) (((f) ) & 0x) > > #define PIXMAN_FORMAT_DEPTH(f)(PIXMAN_FORMAT_A(f) + \ > >PIXMAN_FORMAT_R(f) + \ > >PIXMAN_FORMAT_G(f) + \ > >PIXMAN_FORMAT_B(f)) > > > > > > Right now the format type occupies 8 bits. The highest bit could be > > repurposed as a flag for storing channel depth as bytes instead > > of bits. This way 16-bit and 64-bit per color component will be > > also supported. The limitation of this approach is that the depth > > of every color component has to be a multiple of 8 bits if any of > > them is 16 bits or larger. > > > > I don't feel very comfortable about the fact that some applications > > are using the PIXMAN_FORMAT_A/R/G/B macros and this code gets compiled > > as part of these application binaries. > > > > Are there any other interesting >32bpp formats that we may need > > to support in the future? > > > Doubt it, pixman doesn't have that accuracy currently, but F16 might be > interesting at some point.. > > ~Maarten > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [patch] Gradient dithering into pixman
I get it, the dithering is turned on/off with a Cairo control. I thought you were suggesting that it is turned off if the destination is an image. Quick look at the patch and it seems like this will work, though it is a random dither. You might get much better compression of .png with a patterned dither. I've also had good luck with pseudo-error-diffusion. You keep in static memory an accumulated error (per color, but not really depending on the location of the pixel) and that is the threshold for the random number. This produces a bit more patterning than a straight random generator. You might also try it with no random number generator at all, but you have to preserve the accumulated error so that solid areas that are an integer don't reset it so each adjacent line gets the same pattern. May want to call the "on" setting PIXMAN_DITHERING_GOOD. On the assumption that anybody who wants it on is happy with "good" dithering, and that they may not want to pay for the slowness of "best" dithering. On Tue, Mar 27, 2018 at 2:20 AM, Marc Jeanmougin <m...@jeanmougin.fr> wrote: > Hi, > > Le 27/03/2018 à 02:04, Søren Sandmann a écrit : > > A long time ago I wrote this: > > > > https://lists.freedesktop.org/archives/pixman/2012-July/002175.html > > > > about how dithering could be added to pixman. The basic idea is that > > "dithering" is a property of the destination image, not of the > > gradient. I still think this is the right way to do it. > > Thank you for your input. Would it be possible to use your preferred way > of doing it to my patch ? > > Le 27/03/2018 à 03:47, Bill Spitzak a écrit : > > I don't understand why you would want to disable it when writing .png > > files. There will be banding in the .png file, which I would think is > > worse than the increased size. > Depends. In many cases there is no visible banding, and some people may > not want a x100 filesize increase for it (for files that are entirely > made of gradients). > > > Also kind of fools the user if they did not look at the .png file and > > only at InkScape's display. > We already have such differences for filter quality, and of course it > would be possible to switch it in the display windows for previewing. > > -- > Marc > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [patch] Gradient dithering into pixman
I don't understand why you would want to disable it when writing .png files. There will be banding in the .png file, which I would think is worse than the increased size. Also kind of fools the user if they did not look at the .png file and only at InkScape's display. On Mon, Mar 26, 2018 at 5:04 PM, Søren Sandmannwrote: > Hi, > > A long time ago I wrote this: > > https://lists.freedesktop.org/archives/pixman/2012-July/002175.html > > about how dithering could be added to pixman. The basic idea is that > "dithering" is a property of the destination image, not of the gradient. I > still think this is the right way to do it. > > > Søren > > On Mon, Mar 26, 2018 at 6:37 PM, Marc Jeanmougin > wrote: > >> Hi, >> >> I'm Marc, I'm an Inkscape contributor, and I would like to improve >> pixman by providing a patch to allow gradient dithering, to eliminate >> all gradient banding. This "banding" problem is hugely visible in many >> Inkscape files, and I hope you could help me putting it into pixman, >> then in exposing the functionality into cairo. >> >> I tried to very closely follow the existing code style, and I came up >> with the proof of concept (attached Proof_of_concept.patch ), which >> applies to pixman/master to enable the feature directly. >> >> Since Inkscape also uses pixman to render files when exporting to png, >> and that kind of dithering affects very significantly the file size (PNG >> compression algorithm and randomness don't mix well) we also need a >> switch to enable/disable it "at will". >> >> I tried to understand how it could be done by looking at >> pixman_image_set_* functions and finally came up with the other patch >> attached 0001-Adds-a-gradient-dithering-function-to-pixman.patch (which >> cannot directly be tested, since it would require changes in cairo to >> call pixman_image_set_dithering, but changing "gradient->dithering = >> PIXMAN_DITHERING_NONE;" to _BEST in pixman-image.c works) >> >> Thanks for any help in merging this, or any pointer to how to improve it, >> >> -- >> Marc >> >> ___ >> Pixman mailing list >> Pixman@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/pixman >> >> > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 02/14] Add new test of filter reduction from BILINEAR to NEAREST
On Fri, Apr 29, 2016 at 4:55 AM, Oded Gabbaywrote: > On Tue, Apr 12, 2016 at 5:36 AM, Søren Sandmann Pedersen > wrote: > > This new test tests a bunch of bilinear downscalings, where many have > > a transformation such that the BILINEAR filter can be reduced to > > NEAREST (and many don't). > > > > A CRC32 is computed for all the resulting images and compared to a > > known-good value for both 4-bit and 7-bit interpolation. > > > > V2: Remove leftover comment, some minor formatting fixes, use a > > timestamp as the PRNG seed. > > > > Signed-off-by: Søren Sandmann > > --- > > test/Makefile.sources| 1 + > > test/filter-reduction-test.c | 112 > +++ > > 2 files changed, 113 insertions(+) > > create mode 100644 test/filter-reduction-test.c > > > > diff --git a/test/Makefile.sources b/test/Makefile.sources > > index 5d55e67..0a56231 100644 > > --- a/test/Makefile.sources > > +++ b/test/Makefile.sources > > @@ -21,6 +21,7 @@ TESTPROGRAMS = \ > > gradient-crash-test \ > > pixel-test\ > > matrix-test \ > > + filter-reduction-test \ > > composite-traps-test \ > > region-contains-test \ > > glyph-test\ > > diff --git a/test/filter-reduction-test.c b/test/filter-reduction-test.c > > new file mode 100644 > > index 000..705fa4b > > --- /dev/null > > +++ b/test/filter-reduction-test.c > > @@ -0,0 +1,112 @@ > > +#include > > +#include > > +#include "utils.h" > > + > > +static const pixman_fixed_t entries[] = > > +{ > > +pixman_double_to_fixed (-1.0), > > +pixman_double_to_fixed (-0.5), > > +pixman_double_to_fixed (-1/3.0), > > +pixman_double_to_fixed (0.0), > > +pixman_double_to_fixed (0.5), > > +pixman_double_to_fixed (1.0), > > +pixman_double_to_fixed (1.5), > > +pixman_double_to_fixed (2.0), > > +pixman_double_to_fixed (3.0), > > +}; > > + > > +#define SIZE 12 > > + > > +static uint32_t > > +test_scale (const pixman_transform_t *xform, uint32_t crc) > > +{ > > +uint32_t *srcbuf, *dstbuf; > > +pixman_image_t *src, *dest; > > + > > +srcbuf = malloc (SIZE * SIZE * 4); > I admit I didn't look at other tests, but it really caught my eye we > don't check for memory allocation failure here. > > > +prng_randmemset (srcbuf, SIZE * SIZE * 4, 0); > > +src = pixman_image_create_bits ( > > + PIXMAN_a8r8g8b8, SIZE, SIZE, srcbuf, SIZE * 4); > > + > > +dstbuf = malloc (SIZE * SIZE * 4); > same comment > I think it is ok to not look for that in unit tests. They will either crash or fail. > > > +prng_randmemset (dstbuf, SIZE * SIZE * 4, 0); > > +dest = pixman_image_create_bits ( > > + PIXMAN_a8r8g8b8, SIZE, SIZE, dstbuf, SIZE * 4); > > + > > +pixman_image_set_transform (src, xform); > > +pixman_image_set_repeat (src, PIXMAN_REPEAT_NORMAL); > > +pixman_image_set_filter (src, PIXMAN_FILTER_BILINEAR, NULL, 0); > > + > > +image_endian_swap (src); > > +image_endian_swap (dest); > > + > > +pixman_image_composite (PIXMAN_OP_SRC, > > + src, NULL, dest, > > + 0, 0, 0, 0, 0, 0, > > + SIZE, SIZE); > > + > > +crc = compute_crc32_for_image (crc, dest); > > + > > +pixman_image_unref (src); > > +pixman_image_unref (dest); > > + > > +free (srcbuf); > > +free (dstbuf); > > + > > +return crc; > > +} > > + > > +#if BILINEAR_INTERPOLATION_BITS == 7 > > +#define CHECKSUM 0x02169677 > > +#elif BILINEAR_INTERPOLATION_BITS == 4 > > +#define CHECKSUM 0xE44B29AC > > +#else > > +#define CHECKSUM 0x > > +#endif > > + > > +int > > +main (int argc, const char *argv[]) > > +{ > > +const pixman_fixed_t *end = entries + ARRAY_LENGTH (entries); > > +const pixman_fixed_t *t0, *t1, *t2, *t3, *t4, *t5; > > +uint32_t crc = 0; > > + > > +prng_srand (0x56EA1DBD); > > + > > +for (t0 = entries; t0 < end; ++t0) > > +{ > > + for (t1 = entries; t1 < end; ++t1) > > + { > > + for (t2 = entries; t2 < end; ++t2) > > + { > > + for (t3 = entries; t3 < end; ++t3) > > + { > > + for (t4 = entries; t4 < end; ++t4) > > + { > > + for (t5 = entries; t5 < end; ++t5) > > + { > > + pixman_transform_t xform = { > > + { { *t0, *t1, *t2 }, > > + { *t3, *t4, *t5 }, > > + { 0, 0, pixman_fixed_1 } } > > + }; > > + > > + crc = test_scale (, crc); > > + } > > + } > > + } > > + } > > +
Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)
If the comparison fails, the returned values are modulus fixed_16_16, rather than clamped. I think that gives a lot less possibilities for the calling code to recover from or simulate the results of the out-of-range value. On Fri, Apr 29, 2016 at 6:33 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 29 April 2016 at 11:35, Pekka Paalanen <ppaala...@gmail.com> wrote: > > On Fri, 29 Apr 2016 10:15:37 +0100 > > Emil Velikov <emil.l.veli...@gmail.com> wrote: > > > >> On 27 April 2016 at 18:46, Bill Spitzak <spit...@gmail.com> wrote: > >> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaala...@gmail.com> > wrote: > >> >> > >> >> On Wed, 27 Apr 2016 09:56:44 +0100 > >> >> Emil Velikov <emil.l.veli...@gmail.com> wrote: > >> >> > >> >> > On 26 April 2016 at 19:12, Bill Spitzak <spit...@gmail.com> wrote: > >> >> > > The old code is comparing pixman_fixed_48_16_t values to > >> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation > of > >> >> > > overflow > >> >> > > values. > >> >> > > > >> >> > Indeed it does. I'll grep more before asking silly questions ;-) > >> >> > > >> >> > > It would probably be better to clamp these overflowed values, > like > >> >> > > pixman_transform_point_31_16 is doing to clamp to the > >> >> > > pixman_fixed_48_16 > >> >> > > result. Right now the result is an odd mix of clamping and > modulus. A > >> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values > would be > >> >> > > even > >> >> > > better. > >> >> > > > >> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon. > >> >> > >> >> Wasn't the point of the boolean return from these functions to tell > the > >> >> caller to drop what it is doing because it cannot be done properly? > >> > > >> > > >> > Dropping a fill is a lot worse than trying to simulate it using the > clamped > >> > path. It will produce a wrong result if one of the edges connected to > a > >> > clamped point passes through the clip, but this often does not > happen, and > >> > the transition to the wrong result is gradual as the point moves > outside the > >> > clamped region. > >> > > >> > More importantly the caller cannot do anything with the return values > right > >> > now, as they are modulus MAX_16_16+1. Even the direction they are in > is > >> > lost. > >> > > >> I think that keeping the user provided memory as-is when the function > >> does not succeed is a good idea. > >> Afaics currently the contents get overwritten regardless of the result. > >> > >> This is what you guys were on about, right ? Or perhaps you're > >> thinking about spinning v2 of the function with different > >> signature/behaviour ? > > > > Hi Emil, > > > > I think the conclusion was that the comparisons are not useless, and > > this patch should be dropped. You noted it yourself that this patch > > causes a regression in the test suite. > > > Fully agree on both points. Just trying to understand what you and > Bill are talking about and suggest that if one changes things, would > be nice to avoid "feeding garbage" back to the user [on error]. > > Thanks > Emil > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)
On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Wed, 27 Apr 2016 09:56:44 +0100 > Emil Velikov <emil.l.veli...@gmail.com> wrote: > > > On 26 April 2016 at 19:12, Bill Spitzak <spit...@gmail.com> wrote: > > > The old code is comparing pixman_fixed_48_16_t values to > > > pixman_fixed_16_16_t values, thus it is checking for truncation of > overflow > > > values. > > > > > Indeed it does. I'll grep more before asking silly questions ;-) > > > > > It would probably be better to clamp these overflowed values, like > > > pixman_transform_point_31_16 is doing to clamp to the > pixman_fixed_48_16 > > > result. Right now the result is an odd mix of clamping and modulus. A > > > rewrite to go directly to clamped pixman_fixed_16_16 values would be > even > > > better. > > > > > Sounds like a plan. Sadly I doubt I'll get to it any time soon. > > Wasn't the point of the boolean return from these functions to tell the > caller to drop what it is doing because it cannot be done properly? > Dropping a fill is a lot worse than trying to simulate it using the clamped path. It will produce a wrong result if one of the edges connected to a clamped point passes through the clip, but this often does not happen, and the transition to the wrong result is gradual as the point moves outside the clamped region. More importantly the caller cannot do anything with the return values right now, as they are modulus MAX_16_16+1. Even the direction they are in is lost. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)
The old code is comparing pixman_fixed_48_16_t values to pixman_fixed_16_16_t values, thus it is checking for truncation of overflow values. It would probably be better to clamp these overflowed values, like pixman_transform_point_31_16 is doing to clamp to the pixman_fixed_48_16 result. Right now the result is an odd mix of clamping and modulus. A rewrite to go directly to clamped pixman_fixed_16_16 values would be even better. On Tue, Apr 26, 2016 at 10:59 AM, Petr Kobalíčekwrote: > Doesn't this check for NaNs? > > On Sun, Apr 24, 2016 at 8:22 PM, Emil Velikov > wrote: > >> With commit ed39992564b "Use pixman_transform_point_31_16() from >> pixman_transform_point()" we added some strange hunks. >> >> Namely: we copy the data from the internal storage to the user vector >> only to compare them immediately after. >> >> Cc: Siarhei Siamashka >> --- >> >> Siarhei, what is the intent with the original commit ? Any ideas why >> things crash ? >> >> Seemingly this can be dropped/replaced with TRUE, yet it causes one of >> the tests () to segfault in the optimised SSE2 codepath - >> scaled_bilinear_scanline_sse2___SRC. >> >> BILINEAR_INTERPOLATE_ONE_PIXEL -> BILINEAR_INTERPOLATE_ONE_PIXEL_HELPER >> >> Regards, >> Emil >> --- >> >> pixman/pixman-matrix.c | 8 ++-- >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/pixman/pixman-matrix.c b/pixman/pixman-matrix.c >> index 65b3d32..117015b 100644 >> --- a/pixman/pixman-matrix.c >> +++ b/pixman/pixman-matrix.c >> @@ -393,9 +393,7 @@ pixman_transform_point_3d (const struct >> pixman_transform *transform, >> vector->vector[1] = tmp.v[1]; >> vector->vector[2] = tmp.v[2]; >> >> -return vector->vector[0] == tmp.v[0] && >> - vector->vector[1] == tmp.v[1] && >> - vector->vector[2] == tmp.v[2]; >> +return TRUE; >> } >> >> PIXMAN_EXPORT pixman_bool_t >> @@ -414,9 +412,7 @@ pixman_transform_point (const struct pixman_transform >> *transform, >> vector->vector[1] = tmp.v[1]; >> vector->vector[2] = tmp.v[2]; >> >> -return vector->vector[0] == tmp.v[0] && >> - vector->vector[1] == tmp.v[1] && >> - vector->vector[2] == tmp.v[2]; >> +return TRUE; >> } >> >> PIXMAN_EXPORT pixman_bool_t >> -- >> 2.8.0 >> >> ___ >> Pixman mailing list >> Pixman@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/pixman >> > > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 12/14] pixman-filter: Fix several issues related to normalization
The attached patch changes the normalization to the version I was suggesting. I added some print statements and discovered that the "residual error" was always zero, after some analysis I figured out that this version will always produce a set of values that sum to pixman_fixed_1 (unless the filter is unrealistically wide, far larger than 2^20, to get double precision errors to sum large enough to change this). So I removed the new_total and residual error and left a comment in there. I am having trouble with IMPULSE.BOX producing values that are all equal or very close together. Do you know of a way to get gnuplot to include y==0 as otherwise the plots are very misleading unless you read the axis values. Also this produces "Warning: empty y range [1:1], adjusting to [0.99:1.01]" messages, possibly these are not an actual error. patch Description: Binary data ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/14] pixman-image: Added enable-gnuplot config to view filters in gnuplot
On 04/12/2016 01:55 PM, Søren Sandmann wrote: This series is available as a git repository here: https://cgit.freedesktop.org/~sandmann/pixman/log/?h=spitzak-for-master Not having much luck with that url: fatal: repository 'https://cgit.freedesktop.org/~sandmann/pixman/log/?h=spitzak-for-master/' not found ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 13/14] pixman-filter: Nested polynomial for cubic
Because the integral function is relying on the range being clamped to the non-zero portion for impulse filters and for box, and range checks are missing for some of the other filter functions, I would be tempted to remove the range check from here, and also from the impulse function (it would return 1 for all x). Oded did not like that idea but I wonder if you have any opinion. On Mon, Apr 11, 2016 at 7:36 PM, Søren Sandmann Pedersen < soren.sandm...@gmail.com> wrote: > From: Bill Spitzak <spit...@gmail.com> > > v11: Restored range checks > > Signed-off-by: Bill Spitzak <spit...@gmail.com> > Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> > --- > pixman/pixman-filter.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > index 4abd05f..db4ab6e 100644 > --- a/pixman/pixman-filter.c > +++ b/pixman/pixman-filter.c > @@ -109,14 +109,16 @@ general_cubic (double x, double B, double C) > > if (ax < 1) > { > - return ((12 - 9 * B - 6 * C) * ax * ax * ax + > - (-18 + 12 * B + 6 * C) * ax * ax + (6 - 2 * B)) / 6; > + return (((12 - 9 * B - 6 * C) * ax + > +(-18 + 12 * B + 6 * C)) * ax * ax + > + (6 - 2 * B)) / 6; > } > -else if (ax >= 1 && ax < 2) > +else if (ax < 2) > { > - return ((-B - 6 * C) * ax * ax * ax + > - (6 * B + 30 * C) * ax * ax + (-12 * B - 48 * C) * > - ax + (8 * B + 24 * C)) / 6; > + return -B - 6 * C) * ax + > + (6 * B + 30 * C)) * ax + > +(-12 * B - 48 * C)) * ax + > + (8 * B + 24 * C)) / 6; > } > else > { > -- > 1.7.11.7 > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 12/14] pixman-filter: Fix several issues related to normalization
Excellent idea to put the error diffusion into the division. Just a bit of cleanup and changes for some suspected bugs (that are probably invisible but might as well get them): On Mon, Apr 11, 2016 at 7:36 PM, Søren Sandmann Pedersen < soren.sandm...@gmail.com> wrote: > There are a few bugs in the current normalization code > > (1) The normalization is based on the sum of the *floating point* > values generated by integral(). But in order to get the sum to be > close to pixman_fixed_1, the sum of the rounded fixed point values > should be used. > > (2) The multiplications in the normalization loops often round the > same way, so the residual error can fairly large. > > (3) The residual error is added to the sample located at index > (width - width / 2), which is not the midpoint for odd widths (and > for width 1 is in fact outside the array). > > This patch fixes these issues by (1) using the sum of the fixed point > values as the total to divide by, (2) doing error diffusion in the > normalization loop, and (3) putting any residual error (which is now > guaranteed to be less than pixman_fixed_e) at the first sample, which > is the only one that didn't get any error diffused into it. > > Signed-off-by: Søren Sandmann> --- > pixman/pixman-filter.c | 23 +++ > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > index 32aaa9a..4abd05f 100644 > --- a/pixman/pixman-filter.c > +++ b/pixman/pixman-filter.c > @@ -247,7 +247,7 @@ create_1d_filter (int width, > double frac = step / 2.0 + i * step; > pixman_fixed_t new_total; > int x, x1, x2; > - double total; > + double total, e; > I think total should be a pixman_fixed_t as that is what it is summing. > /* Sample convolution of reconstruction and sampling > * filter. See rounding.txt regarding the rounding > @@ -278,24 +278,31 @@ create_1d_filter (int width, > ihigh - ilow); > } > > - total += c; > -*p++ = (pixman_fixed_t)(c * 65536.0 + 0.5); > +*p = (pixman_fixed_t)floor (c * 65536.0 + 0.5); > floor probably is a good idea to make negative filter entries work. Is there a macro that does this conversion? + total += *p; > + p++; > } > Might want to skip the normalize if total==pixman_fixed_1, though perhaps a test to see how often that happens would be informative. > > - /* Normalize */ > + /* Normalize, with error diffusion */ > p -= width; > -total = 1 / total; > +total = 65536.0 / total; > Remove the division so total can be a pixman_fixed_t. > new_total = 0; > + e = 0.0; > Change this to 0.5 > for (x = x1; x < x2; ++x) > { > - pixman_fixed_t t = (*p) * total + 0.5; > + double v = (*p) * total + e; Change to this so total can be a pixman_fixed_t: double v = (*p) * (double)(pixman_fixed_1) / total + e; + pixman_fixed_t t = floor (v + 0.5); > Change this to just floor(v). The 0.5 factor is incorporated into e. This version is in effect adding .5 to all the samples, though that is compensated for somewhat by floor in effect subtracting .5, but imagine what happens if e > 0.5. This bug was in Nuke for many years btw. > + e = v - t; > new_total += t; > *p++ = t; > } > > - if (new_total != pixman_fixed_1) > - *(p - width / 2) += (pixman_fixed_1 - new_total); > + /* pixman_fixed_e's worth of error may remain; put it > +* at the first sample, since that is the only one that > +* hasn't had any error diffused into it. > +*/ > + *(p - width) += pixman_fixed_1 - new_total; > I'm not absolutely convinced that the first sample is best. Dumping this on the central pixel may be better because that value is larger so the relative change is smaller: *(p - (width+1)/2) += pixman_fixed_1 - new_total; > } > } > > -- > 1.7.11.7 > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/14] pixman-image: Added enable-gnuplot config to view filters in gnuplot
On Tue, Apr 12, 2016 at 1:55 PM, Søren Sandmannwrote: > > 1-wide filters - looks triangular, but a 1-wide box would be more >>>accurate >>> >> >> Because you are not plotting the two dummy points at (0,±width/2), a >> 1-wide filter is actually just a single point. >> >> You may be right that leaving the dummy points off the plot may make it >> easier to figure out what is going on. >> > > Okay, I will update the comment. > > I don't like to make up fake data points, but maybe I should add > [x=-width/2:width/2] to the gnuplot command line. > Yes that sounds like a very good idea. > > diff --git a/pixman/rounding.txt b/pixman/rounding.txt >> >>> index b52b084..1c00019 100644 >>> --- a/pixman/rounding.txt >>> +++ b/pixman/rounding.txt >>> @@ -160,6 +160,7 @@ which means the contents of the matrix corresponding >>> to (frac) should >>> contain width samplings of the function, with the first sample at: >>> >>> floor (frac - (width - 1) / 2.0 - e) + 0.5 - frac >>> + = ceil (frac - width / 2.0 - 0.5) + 0.5 - frac >>> >> >> Unfortunately this produces numbers that don't agree with the filter >> generator or filtering code. >> >> For a width==4 filter with n_phases==1, the generator seems to produce >> values at -1, 0, 1, 2, so the first sample is at -1. It also appears the >> filtering sampler is using the same rule, otherwise these filters would >> produce an obvious shift in the image. >> > > If you add printf's on top of this series > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > index 176dfae..4a8743e 100644 > --- a/pixman/pixman-filter.c > +++ b/pixman/pixman-filter.c > @@ -129,12 +129,15 @@ general_cubic (double x, double B, double C) > static double > cubic_kernel (double x) > { > + double v = general_cubic (x, 1/3.0, 1/3.0); > + > + printf ("cubic(%f) => %f\n", x, v); > /* This is the Mitchell-Netravali filter. > * > * (0.0, 0.5) would give us the Catmull-Rom spline, > * but that one seems to be indistinguishable from Lanczos2. > */ > -return general_cubic (x, 1/3.0, 1/3.0); > + return v; > } > > and run scale with Reconstruction=Cubic, Sample=Impulse, and Subsample=0, > it prints > > cubic(-2.00) => 0.00 > cubic(-1.00) => 0.06 > cubic(0.00) => 0.89 > cubic(1.00) => 0.06 > > so the filter generator *does* generate values at [ -2, -1, 0, 1 ]. And as > mentioned, the sampling code, if given the value 17.5 will convolve with > the pixels at 15.5, 16.5, 17.5, 18.5, so I'm pretty sure this matrix is > correct. > > (I suspect your changes to the integral() arguments caused it to generate > different values, so you should check without them included. > Looks like you are correct, toggling between this and nearest shows that the filtering code is reading the arrays this way. > > This series is available as a git repository here: > > > https://cgit.freedesktop.org/~sandmann/pixman/log/?h=spitzak-for-master > ) > > > Søren > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/14] pixman-image: Added enable-gnuplot config to view filters in gnuplot
On Mon, Apr 11, 2016 at 7:36 PM, Søren Sandmann Pedersen < soren.sandm...@gmail.com> wrote: > From: Bill Spitzak <spit...@gmail.com> > > If enable-gnuplot is configured, then you can pipe the output of a > pixman-using program to gnuplot and get a continuously-updated plot of > the horizontal filter. This works well with demos/scale to test the > filter generation. > > The plot is all the different subposition filters shuffled > together. This is misleading in a few cases: > > IMPULSE.BOX - goes up and down as the subfilters have different > numbers of non-zero samples > > IMPULSE.TRIANGLE - somewhat crooked for the same reason > > 1-wide filters - looks triangular, but a 1-wide box would be more >accurate > Because you are not plotting the two dummy points at (0,±width/2), a 1-wide filter is actually just a single point. You may be right that leaving the dummy points off the plot may make it easier to figure out what is going on. > Changes by Søren: Rewrote the pixman-filter.c part to > - make it generate correct coordinates > - add a comment on how coordinates are generated > - in rounding.txt, add a ceil() variant of the first-sample >formula > - make the gnuplot output slightly prettier > > v7: First time this ability was included > > v8: Use config option > Moved code to the filter generator > Modified scale demo to not call filter generator a second time. > > v10: Only print if successful generation of plots > Use #ifdef, not #if > > v11: small whitespace fixes > > Signed-off-by: Bill Spitzak <spit...@gmail.com> > Signed-off-by: Søren Sandmann <soren.sandm...@gmail.com> > --- > configure.ac | 13 ++ > pixman/pixman-filter.c | 115 > + > pixman/rounding.txt| 1 + > 3 files changed, 129 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 6b2134e..e833e45 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -834,6 +834,19 @@ fi > AC_SUBST(PIXMAN_TIMERS) > > dnl === > +dnl gnuplot > + > +AC_ARG_ENABLE(gnuplot, > + [AC_HELP_STRING([--enable-gnuplot], > + [enable output of filters that can be piped to gnuplot > [default=no]])], > + [enable_gnuplot=$enableval], [enable_gnuplot=no]) > + > +if test $enable_gnuplot = yes ; then > + AC_DEFINE(PIXMAN_GNUPLOT, 1, [enable output that can be piped to > gnuplot]) > +fi > +AC_SUBST(PIXMAN_GNUPLOT) > + > +dnl === > dnl GTK+ > > AC_ARG_ENABLE(gtk, > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > index b2bf53f..af46a43 100644 > --- a/pixman/pixman-filter.c > +++ b/pixman/pixman-filter.c > @@ -297,6 +297,117 @@ create_1d_filter (int *width, > return params; > } > > +#ifdef PIXMAN_GNUPLOT > + > +/* If enable-gnuplot is configured, then you can pipe the output of a > + * pixman-using program to gnuplot and get a continuously-updated plot > + * of the horizontal filter. This works well with demos/scale to test > + * the filter generation. > + * > + * The plot is all the different subposition filters shuffled > + * together. This is misleading in a few cases: > + * > + * IMPULSE.BOX - goes up and down as the subfilters have different > + * numbers of non-zero samples > + * IMPULSE.TRIANGLE - somewhat crooked for the same reason > + * 1-wide filters - looks triangular, but a 1-wide box would be more > + * accurate > + */ > +static void > +gnuplot_filter (int width, int n_phases, const pixman_fixed_t* p) > +{ > +double step; > +int i, j; > +int first; > + > +step = 1.0 / n_phases; > + > +printf ("set style line 1 lc rgb '#0060ad' lt 1 lw 0.5 pt 7 pi 1 ps > 0.5\n"); > +printf ("plot '-' with linespoints ls 1\n"); > + > +/* The position of the first sample of the phase corresponding to > + * frac is given by: > + * > + * ceil (frac - width / 2.0 - 0.5) + 0.5 - frac > + * > + * We have to find the frac that minimizes this expression. > + * > + * For odd widths, we have > + * > + * ceil (frac - width / 2.0 - 0.5) + 0.5 - frac > + * = ceil (frac) + K - frac > + * = 1 + K - frac > + * > + * for some K, so this is minimized when frac is maximized and > + * strictly growing with frac. So for odd widths, we can simply > + * start at the last phase and go backwards. > + * > + * For even widths, we have > + * >
Re: [Pixman] [PATCH 03/14] More general BILINEAR=>NEAREST reduction
Only minor comments on the large nearest patch at the bottom On 04/11/2016 07:36 PM, Søren Sandmann Pedersen wrote: Generalize and simplify the code that reduces BILINEAR to NEAREST so that the reduction happens for all affine transformations where t00...t12 are integers and (t00 + t01) and (t10 + t11) are both odd. This is a sufficient condition for the resulting transformed coordinates to be exactly at the center of a pixel so that BILINEAR becomes identical to NEAREST. V2: Address some comments by Bill Spitzak Signed-off-by: Søren Sandmann <soren.sandm...@gmail.com> --- pixman/pixman-image.c | 66 +-- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c index 1ff1a49..681864e 100644 --- a/pixman/pixman-image.c +++ b/pixman/pixman-image.c @@ -335,37 +335,47 @@ compute_image_info (pixman_image_t *image) { flags |= FAST_PATH_NEAREST_FILTER; } - else if ( - /* affine and integer translation components in matrix ... */ - ((flags & FAST_PATH_AFFINE_TRANSFORM) && -!pixman_fixed_frac (image->common.transform->matrix[0][2] | -image->common.transform->matrix[1][2])) && - ( - /* ... combined with a simple rotation */ - (flags & (FAST_PATH_ROTATE_90_TRANSFORM | - FAST_PATH_ROTATE_180_TRANSFORM | - FAST_PATH_ROTATE_270_TRANSFORM)) || - /* ... or combined with a simple non-rotated translation */ - (image->common.transform->matrix[0][0] == pixman_fixed_1 && -image->common.transform->matrix[1][1] == pixman_fixed_1 && -image->common.transform->matrix[0][1] == 0 && -image->common.transform->matrix[1][0] == 0) - ) - ) + else if (flags & FAST_PATH_AFFINE_TRANSFORM) { - /* FIXME: there are some affine-test failures, showing that -* handling of BILINEAR and NEAREST filter is not quite -* equivalent when getting close to 32K for the translation -* components of the matrix. That's likely some bug, but for -* now just skip BILINEAR->NEAREST optimization in this case. + /* Suppose the transform is +* +*[ t00, t01, t02 ] +*[ t10, t11, t12 ] +*[ 0, 0, 1 ] +* +* and the destination coordinates are (n + 0.5, m + 0.5). Then +* the transformed x coordinate is: +* +* tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02 +*= t00 * n + t01 * m + t02 + (t00 + t01) * 0.5 +* +* which implies that if t00, t01 and t02 are all integers +* and (t00 + t01) is odd, then tx will be an integer plus 0.5, +* which means a BILINEAR filter will reduce to NEAREST. The same +* applies in the y direction */ - pixman_fixed_t magic_limit = pixman_int_to_fixed (3); - if (image->common.transform->matrix[0][2] <= magic_limit && - image->common.transform->matrix[1][2] <= magic_limit && - image->common.transform->matrix[0][2] >= -magic_limit && - image->common.transform->matrix[1][2] >= -magic_limit) + pixman_fixed_t (*t)[3] = image->common.transform->matrix; + + if ((pixman_fixed_frac ( +t[0][0] | t[0][1] | t[0][2] | +t[1][0] | t[1][1] | t[1][2]) == 0) && + (pixman_fixed_to_int ( + (t[0][0] + t[0][1]) & (t[1][0] + t[1][1])) % 2) == 1) { - flags |= FAST_PATH_NEAREST_FILTER; + /* FIXME: there are some affine-test failures, showing that +* handling of BILINEAR and NEAREST filter is not quite +* equivalent when getting close to 32K for the translation +* components of the matrix. That's likely some bug, but for +* now just skip BILINEAR->NEAREST optimization in this case. +*/ May be a good idea to point out that the bug is (probably) in BILINEAR, not in NEAREST. Also you think this may have been fixed so this could be removed, but that would be a different patch. + pixman_fixed_t magic_limit = pixman_int_to_fixed (3); + if (image->common.transform->matrix[0][2] <= magic_limit && + image->common.transform->matrix[1][2] <= magic_limit && + image->common.transform-&
Re: [Pixman] [PATCH 02/14] Add new test of filter reduction from BILINEAR to NEAREST
Reviewed-by: Bill Spitzak <spit...@gmail.com> On 04/11/2016 07:36 PM, Søren Sandmann Pedersen wrote: This new test tests a bunch of bilinear downscalings, where many have a transformation such that the BILINEAR filter can be reduced to NEAREST (and many don't). A CRC32 is computed for all the resulting images and compared to a known-good value for both 4-bit and 7-bit interpolation. V2: Remove leftover comment, some minor formatting fixes, use a timestamp as the PRNG seed. Signed-off-by: Søren Sandmann <soren.sandm...@gmail.com> --- test/Makefile.sources| 1 + test/filter-reduction-test.c | 112 +++ 2 files changed, 113 insertions(+) create mode 100644 test/filter-reduction-test.c diff --git a/test/Makefile.sources b/test/Makefile.sources index 5d55e67..0a56231 100644 --- a/test/Makefile.sources +++ b/test/Makefile.sources @@ -21,6 +21,7 @@ TESTPROGRAMS = \ gradient-crash-test \ pixel-test\ matrix-test \ + filter-reduction-test \ composite-traps-test \ region-contains-test \ glyph-test\ diff --git a/test/filter-reduction-test.c b/test/filter-reduction-test.c new file mode 100644 index 000..705fa4b --- /dev/null +++ b/test/filter-reduction-test.c @@ -0,0 +1,112 @@ +#include +#include +#include "utils.h" + +static const pixman_fixed_t entries[] = +{ +pixman_double_to_fixed (-1.0), +pixman_double_to_fixed (-0.5), +pixman_double_to_fixed (-1/3.0), +pixman_double_to_fixed (0.0), +pixman_double_to_fixed (0.5), +pixman_double_to_fixed (1.0), +pixman_double_to_fixed (1.5), +pixman_double_to_fixed (2.0), +pixman_double_to_fixed (3.0), +}; + +#define SIZE 12 + +static uint32_t +test_scale (const pixman_transform_t *xform, uint32_t crc) +{ +uint32_t *srcbuf, *dstbuf; +pixman_image_t *src, *dest; + +srcbuf = malloc (SIZE * SIZE * 4); +prng_randmemset (srcbuf, SIZE * SIZE * 4, 0); +src = pixman_image_create_bits ( + PIXMAN_a8r8g8b8, SIZE, SIZE, srcbuf, SIZE * 4); + +dstbuf = malloc (SIZE * SIZE * 4); +prng_randmemset (dstbuf, SIZE * SIZE * 4, 0); +dest = pixman_image_create_bits ( + PIXMAN_a8r8g8b8, SIZE, SIZE, dstbuf, SIZE * 4); + +pixman_image_set_transform (src, xform); +pixman_image_set_repeat (src, PIXMAN_REPEAT_NORMAL); +pixman_image_set_filter (src, PIXMAN_FILTER_BILINEAR, NULL, 0); + +image_endian_swap (src); +image_endian_swap (dest); + +pixman_image_composite (PIXMAN_OP_SRC, + src, NULL, dest, + 0, 0, 0, 0, 0, 0, + SIZE, SIZE); + +crc = compute_crc32_for_image (crc, dest); + +pixman_image_unref (src); +pixman_image_unref (dest); + +free (srcbuf); +free (dstbuf); + +return crc; +} + +#if BILINEAR_INTERPOLATION_BITS == 7 +#define CHECKSUM 0x02169677 +#elif BILINEAR_INTERPOLATION_BITS == 4 +#define CHECKSUM 0xE44B29AC +#else +#define CHECKSUM 0x +#endif + +int +main (int argc, const char *argv[]) +{ +const pixman_fixed_t *end = entries + ARRAY_LENGTH (entries); +const pixman_fixed_t *t0, *t1, *t2, *t3, *t4, *t5; +uint32_t crc = 0; + +prng_srand (0x56EA1DBD); + +for (t0 = entries; t0 < end; ++t0) +{ + for (t1 = entries; t1 < end; ++t1) + { + for (t2 = entries; t2 < end; ++t2) + { + for (t3 = entries; t3 < end; ++t3) + { + for (t4 = entries; t4 < end; ++t4) + { + for (t5 = entries; t5 < end; ++t5) + { + pixman_transform_t xform = { + { { *t0, *t1, *t2 }, + { *t3, *t4, *t5 }, + { 0, 0, pixman_fixed_1 } } + }; + + crc = test_scale (, crc); + } + } + } + } + } +} + +if (crc != CHECKSUM) +{ + printf ("filter-reduction-test failed! (checksum=0x%08X, expected 0x%08X)\n", crc, CHECKSUM); + return 1; +} +else +{ + printf ("filter-reduction-test passed (checksum=0x%08X)\n", crc); + return 0; +} +} ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 01/14] pixman-fast-path.c: Pick NEAREST affine fast paths before BILINEAR ones
I can confirm this fixes the problem and allows multiple fast path flags to be set. Reviewed-by: Bill Spitzak <spit...@gmail.com> On 04/11/2016 07:36 PM, Søren Sandmann Pedersen wrote: When a BILINEAR filter is reduced to NEAREST, it is possible for both types of fast paths to run; in this case, the NEAREST ones should be preferred as that is the simpler filter. Signed-off-by: Soren Sandmann <soren.sandm...@gmail.com> --- pixman/pixman-fast-path.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pixman/pixman-fast-path.c b/pixman/pixman-fast-path.c index 53d4a1f..b4daa26 100644 --- a/pixman/pixman-fast-path.c +++ b/pixman/pixman-fast-path.c @@ -3258,9 +3258,9 @@ static const pixman_iter_info_t fast_iters[] = }, #define AFFINE_FAST_PATHS(name, format, repeat) \ -SEPARABLE_CONVOLUTION_AFFINE_FAST_PATH(name, format, repeat) \ +NEAREST_AFFINE_FAST_PATH(name, format, repeat) \ BILINEAR_AFFINE_FAST_PATH(name, format, repeat) \ -NEAREST_AFFINE_FAST_PATH(name, format, repeat) +SEPARABLE_CONVOLUTION_AFFINE_FAST_PATH(name, format, repeat) AFFINE_FAST_PATHS (pad_a8r8g8b8, a8r8g8b8, PAD) AFFINE_FAST_PATHS (none_a8r8g8b8, a8r8g8b8, NONE) ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
On Mon, Apr 11, 2016 at 12:35 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Mon, Apr 11, 2016 at 2:43 PM, Bill Spitzak <spit...@gmail.com> wrote: > > >> I feel this can be fixed. It is already correct for subsample_bits==0. >> Since both the filter generator and filtering code would be changed in the >> same version, only programs that generate their own filters would actually >> be incompatible. And as you point out the error is very tiny for large >> numbers of subsamples, and Cairo is already using an excessively large >> number of subsamples (likely because I was trying to remove the difference >> between identity filters and nearest filtering, and did not realize this >> was the underlying problem). >> > > It is technically an ABI break, and cairo 1.14 did ship with a > copied-and-pasted filter generator that assumes the current subpixel > positioning. > > But yeah, maybe we can just ignore that, if we make sure a there is a > cairo 1.14.x update that uses pixman_filter_create_separate_convolution() > instead of the copied-and-pasted filter generator. > > Other than that, the fix should be straight-forward enough. > As I wrote that Cairo patch I can state that I think it would be perfectly fine to make this change. The subsamples are set unnaturally high in that patch and hide the problem. Also it is possible my filter generator is producing centered samples so this could be an improvement, need to check. In addition, other than the (abandoned) idea of getting good/best into pixman, this patch series is also designed so that Cairo can use the pixman filter generator rather than it's own. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
Okay the actual bug is that the gnuplot output is wrong for subsample_bits==0. It apparently is correct for other values of subsampling. I will try to get an updated version posted soon. But I very much agree with Owen (and you?) that the current behavior is incorrect and should be fixed exactly as stated. The current version produces different images from fallbacks to impulse, bilinear, or integer-sized box filters, when the filter math says the fallback should produce an identical result. Scaling down by an integer produces a slightly blurry and offset result, when a perfect result could be achieved at the same speed. Increasing the subsamples moves the existing samples, thus may increase artifacts, rather than always reducing them. All of this is not a good situation. I feel this can be fixed. It is already correct for subsample_bits==0. Since both the filter generator and filtering code would be changed in the same version, only programs that generate their own filters would actually be incompatible. And as you point out the error is very tiny for large numbers of subsamples, and Cairo is already using an excessively large number of subsamples (likely because I was trying to remove the difference between identity filters and nearest filtering, and did not realize this was the underlying problem). On Sun, Apr 10, 2016 at 10:01 PM, Søren Sandmannwrote: > > It does look like there is something really wrong. I compared and (except >> for the subsample_bits==0 case) my version produces the same output as the >> current git head. >> >> I think your intention is that there is a sample at offset=0 whether the >> filter width is even or odd. However (except when subsample_bits==0) the >> filter generator makes a symmetric filter for even sizes, with two equal >> samples around the maximum center value. If a sample was at offset==0 then >> it would be unique and larger than all the other samples. >> > > The root of this confusion is probably that when subsample_bits = k, the > subpixel positions used are: > > 0.5 / 2^k, 1.5 / 2^k, ..., (2^k-0.5)/2^k > > For example, for subsample_bits = 2: > > 0.125, 0.375, 0.625, 0.875 > > and for subsample_bits = 0: > > 0.5 > > That is, they are regularly spaced, but centered within the pixel. When > there is an even number of them, this means there will not be a filter > position at 0.5, and therefore no sample at offset 0. And the only case > where number of subpixel locations is odd, is when subsample_bits = 0. > > I'm pretty sure that the existing code gets the filter generation right > for these subpixel positions. > > > [ You can argue that it would be better to use the sampling positions > > 0, 0.25, 0.5, 0.75 > > for subsample_bits = 2, as Owen did here: > > https://lists.cairographics.org/archives/cairo/2014-March/025105.html > > and I agree that that would have been better. ] > > > Søren > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
On 04/09/2016 01:13 PM, Søren Sandmann wrote: On Sat, Apr 9, 2016 at 3:45 PM, Bill Spitzak <spit...@gmail.com <mailto:spit...@gmail.com>> wrote: On 04/03/2016 11:17 AM, Søren Sandmann 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. I have finally checked this and it is producing the correct value in the above example. pos is not the left edge of the range to integrate, it is the center of it, due to the changes I made to the integrate function. No, it really doesn't produce the correct values. If you apply the patch below on top of your series and run demos/scale with subsample set to 0, reconstruct set to 'Cubic', sample set to 'Impulse' scale factor set to 1.0, it prints out this: cubic(1.50) = -0.034722 cubic(0.50) = 0.534722 cubic(-0.50) = 0.534722 cubic(-1.50) = -0.034722 [ -0.035, 0.535, 0.535, -0.035, ] which is not the correct result. The first value should be cubic(-2) = 0. And if you toggle back and forth between subsamples = 0 and 1, you can see the image shift slightly. (The *gnuplot* graph looks correct, but that's because the gnuplot function is also generating the wrong coordinates). It does look like there is something really wrong. I compared and (except for the subsample_bits==0 case) my version produces the same output as the current git head. I think your intention is that there is a sample at offset=0 whether the filter width is even or odd. However (except when subsample_bits==0) the filter generator makes a symmetric filter for even sizes, with two equal samples around the maximum center value. If a sample was at offset==0 then it would be unique and larger than all the other samples. Comparing box+box with subsample_bits==1 with bilinear, it does appear there is an offset and the filter is wrong. It matches exactly (in your version) when subsample_bits==0. The gnuplot output is wrong because I adjusted it to make this output look correct. I then further broke the only case that actually worked (when subsample_bits==0) with this patch, since it did not match the others. I need to look at the actual filter sampling code and at your paper I guess. I think that is doing the right thing, and the generator and gnuplot have to be adjusted to match. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
On 04/03/2016 11:17 AM, Søren Sandmann 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. I have finally checked this and it is producing the correct value in the above example. pos is not the left edge of the range to integrate, it is the center of it, due to the changes I made to the integrate function. However the code is incorrect and only works because n_phases&1 is true only when subsample_bits == 0. The intention was to treat all odd widths the same. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
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.sandm...@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 <spit...@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.sandm...@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, <spit...@gmail.com> wrote: >>> >>>> From: Bill Spitzak <spit...@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 <spit...@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@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/pixman >>>> >>> >>> >> > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 08/22] pixman-filter: rename "scale" to "size" when it is 1/scale
Okay that sounds pretty good. Also 'r' is better than 'i' since 'i' can be mis-read as "input" or "image". 'r' I think is mostly going to be read as "reverse" which actually makes sense. Sorry to be a pain about this but I really find it confusing to use the same term for different numbers. On Sun, Apr 3, 2016 at 9:29 AM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > I guess I can live with 'rscale' (for reciprocal scale). > > > Søren > > On Thu, Mar 24, 2016 at 9:30 PM, Bill Spitzak <spit...@gmail.com> wrote: > >> Would using "iscale" (for inverse scale) work? Another is "tscale" >> because it is the scale from the transform matrix. >> >> I really would like to use two different names for these variables as it >> is really confusing using the same one. >> >> >> On Fri, Mar 18, 2016 at 8:24 AM, Bill Spitzak <spit...@gmail.com> wrote: >> >>> >>> >>> On Thu, Mar 17, 2016 at 10:06 PM, Søren Sandmann < >>> soren.sandm...@gmail.com> wrote: >>> >>>> I suppose it's a little illogical that scale_x and scale_y really are >>>> the reciprocal values of how much the source image should be scaled. I >>>> don't remember exactly what I was thinking, but it might have something >>>> like "this allows you to just pass t[0][0] and t[1][1] if you have a pure >>>> scaling, which avoids a division". There is also kind of precedence in the >>>> Pixman API since the transformation given as in the dst->source direction. >>>> >>>> I don't really like "size" either though. It's not really the size of >>>> the filter that we are specifying; it just happens to be proportional to >>>> it. >>>> >>>> If it comes down to "size" and "scale", I prefer "scale". >>>> >>> >>> I tried "width" but it is not actually the filter width. How about "dx" >>> and "dy" since it is almost always the derivative of the x and y in the >>> input (or du,dv or dtx dtx) >>> >>>> >>>> >>>> Søren >>>> >>>> >>>> On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: >>>> >>>>> From: Bill Spitzak <spit...@gmail.com> >>>>> >>>>> This is to remove some confusion when reading the code. "scale" gets >>>>> larger >>>>> as the picture gets larger, while "size" (ie the size of the filter) >>>>> gets >>>>> smaller. >>>>> >>>>> v14: Removed changes to integral function >>>>> >>>>> Signed-off-by: Bill Spitzak <spit...@gmail.com> >>>>> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >>>>> --- >>>>> pixman/pixman-filter.c | 18 +- >>>>> pixman/pixman.h| 6 +++--- >>>>> 2 files changed, 12 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >>>>> index a29116a..c03a7f6 100644 >>>>> --- a/pixman/pixman-filter.c >>>>> +++ b/pixman/pixman-filter.c >>>>> @@ -221,7 +221,7 @@ static void >>>>> create_1d_filter (int width, >>>>> pixman_kernel_t reconstruct, >>>>> pixman_kernel_t sample, >>>>> - double scale, >>>>> + double size, >>>>> int n_phases, >>>>> pixman_fixed_t *p) >>>>> { >>>>> @@ -251,8 +251,8 @@ create_1d_filter (int width, >>>>> double pos = x + 0.5 - frac; >>>>> double rlow = - filters[reconstruct].width / 2.0; >>>>> double rhigh = rlow + filters[reconstruct].width; >>>>> - double slow = pos - scale * filters[sample].width / 2.0; >>>>> - double shigh = slow + scale * filters[sample].width; >>>>> + double slow = pos - size * filters[sample].width / 2.0; >>>>> + double shigh = slow + size * filters[sample].width; >>>>> double c = 0.0; >>>>> double ilow, ihigh; >>>&g
Re: [Pixman] [PATCH v14 12/22] pixman-filter: fix subsample_bits == 0
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.sandm...@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, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@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 <spit...@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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/pixman >> > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 08/22] pixman-filter: rename "scale" to "size" when it is 1/scale
Would using "iscale" (for inverse scale) work? Another is "tscale" because it is the scale from the transform matrix. I really would like to use two different names for these variables as it is really confusing using the same one. On Fri, Mar 18, 2016 at 8:24 AM, Bill Spitzak <spit...@gmail.com> wrote: > > > On Thu, Mar 17, 2016 at 10:06 PM, Søren Sandmann <soren.sandm...@gmail.com > > wrote: > >> I suppose it's a little illogical that scale_x and scale_y really are the >> reciprocal values of how much the source image should be scaled. I don't >> remember exactly what I was thinking, but it might have something like >> "this allows you to just pass t[0][0] and t[1][1] if you have a pure >> scaling, which avoids a division". There is also kind of precedence in the >> Pixman API since the transformation given as in the dst->source direction. >> >> I don't really like "size" either though. It's not really the size of the >> filter that we are specifying; it just happens to be proportional to it. >> >> If it comes down to "size" and "scale", I prefer "scale". >> > > I tried "width" but it is not actually the filter width. How about "dx" > and "dy" since it is almost always the derivative of the x and y in the > input (or du,dv or dtx dtx) > >> >> >> Søren >> >> >> On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: >> >>> From: Bill Spitzak <spit...@gmail.com> >>> >>> This is to remove some confusion when reading the code. "scale" gets >>> larger >>> as the picture gets larger, while "size" (ie the size of the filter) gets >>> smaller. >>> >>> v14: Removed changes to integral function >>> >>> Signed-off-by: Bill Spitzak <spit...@gmail.com> >>> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >>> --- >>> pixman/pixman-filter.c | 18 +- >>> pixman/pixman.h| 6 +++--- >>> 2 files changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >>> index a29116a..c03a7f6 100644 >>> --- a/pixman/pixman-filter.c >>> +++ b/pixman/pixman-filter.c >>> @@ -221,7 +221,7 @@ static void >>> create_1d_filter (int width, >>> pixman_kernel_t reconstruct, >>> pixman_kernel_t sample, >>> - double scale, >>> + double size, >>> int n_phases, >>> pixman_fixed_t *p) >>> { >>> @@ -251,8 +251,8 @@ create_1d_filter (int width, >>> double pos = x + 0.5 - frac; >>> double rlow = - filters[reconstruct].width / 2.0; >>> double rhigh = rlow + filters[reconstruct].width; >>> - double slow = pos - scale * filters[sample].width / 2.0; >>> - double shigh = slow + scale * filters[sample].width; >>> + double slow = pos - size * filters[sample].width / 2.0; >>> + double shigh = slow + size * filters[sample].width; >>> double c = 0.0; >>> double ilow, ihigh; >>> >>> @@ -262,7 +262,7 @@ create_1d_filter (int width, >>> ihigh = MIN (shigh, rhigh); >>> >>> c = integral (reconstruct, ilow, >>> - sample, 1.0 / scale, ilow - pos, >>> + sample, 1.0 / size, ilow - pos, >>> ihigh - ilow); >>> } >>> >>> @@ -335,12 +335,12 @@ filter_width (pixman_kernel_t reconstruct, >>> pixman_kernel_t sample, double size) >>> } >>> >>> /* Create the parameter list for a SEPARABLE_CONVOLUTION filter >>> - * with the given kernels and scale parameters >>> + * with the given kernels and size parameters >>> */ >>> PIXMAN_EXPORT pixman_fixed_t * >>> pixman_filter_create_separable_convolution (int *n_values, >>> - pixman_fixed_t scale_x, >>> - pixman_fixed_t scale_y, >>> + pixman_fixed_t size_x, >>> + pixman_fixed_t size_y, >>>
Re: [Pixman] [PATCH v14 10/22] pixman-filter: Correct integration with impulse filters
On Mon, Mar 21, 2016 at 7:41 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > > > On Mon, Mar 21, 2016 at 12:29 PM, Bill Spitzak <spit...@gmail.com> wrote: > >> On Sun, Mar 20, 2016 at 11:36 PM, Søren Sandmann < >> soren.sandm...@gmail.com> wrote: >> >>> On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: >>> >>>> From: Bill Spitzak <spit...@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. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 16/22] pixman-filter: distribute normalization error over filter
On Fri, Mar 18, 2016 at 6:54 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> This removes a high-frequency spike in the middle of some filters that is >> caused by math errors all being in the same direction. >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> --- >> pixman/pixman-filter.c | 12 +++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >> index 36dd811..ab62e0a 100644 >> --- a/pixman/pixman-filter.c >> +++ b/pixman/pixman-filter.c >> @@ -282,8 +282,18 @@ create_1d_filter (int width, >> p[x] = t; >> } >> >> + /* Distribute any remaining error over all samples */ >> if (new_total != pixman_fixed_1) >> - p[width / 2] += (pixman_fixed_1 - new_total); >> + { >> + pixman_fixed_t delta = new_total - pixman_fixed_1; >> + pixman_fixed_t t = 0; >> + for (x = 0; x < width; ++x) >> + { >> + pixman_fixed_t new_t = delta * (x + 1) / width; >> + p[x] += new_t - t; >> + t = new_t; >> + } >> + } >> > > I think there is a sign error in this code: delta is new_total - 1, which > is positive when new_total is bigger than 1. But this positive delta is > then added to the samples, making the total even bigger. > Yes I believe you are right. Looks like my mistake there, and you can see the other line that this replaces is in the correct direction so I'm not sure how I managed to do this. This incorrect version still hid the artifact in the filter (a small dip/spike in the middle of large ones for sync filters), so I guess I concluded I got it right. Also, I would write the code like this: > > pixman_fixed_t error = pixman_fixed_1 - new_total; > for (x = 0; x < width; ++x) > { > pixman_fixed_t d = error * (x + 1) / width; > p[x] += d; > error -= d; > } > > to get rid of the temporary and to make it more obvious that there is an > error that is being distributed. > That will not distribute the error evenly. I could just add error*(x+1)/width-error*x/width and avoid the temporary entirely, though I guess there has to be a warning comment that doing the obvious-looking optimization will make it not work. > > Another possibility is to do error diffusion in the /* Normalize */ loop. > I don't think that will work because it needs to take into account the rounding to pixman_fixed_t after the division. > > > Søren > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v14 10/22] pixman-filter: Correct integration with impulse filters
On Sun, Mar 20, 2016 at 11:36 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@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. > > > > Søren > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCHv2 3/3] More general BILINEAR=>NEAREST reduction
Looks good to me On Wed, Mar 16, 2016 at 9:14 PM, Søren Sandmann Pedersen < soren.sandm...@gmail.com> wrote: > Generalize and simplify the code that reduces BILINEAR to NEAREST so > that all the reduction happens for all affine transformations where > t00..t12 are integers and (t00 + t01) and (t10 + t11) are both > odd. This is a sufficient condition for the resulting transformed > coordinates to be exactly at the center of a pixel so that BILINEAR > becomes identical to NEAREST. > > V2: Address some comments by Bill Spitzak > > Signed-off-by: Søren Sandmann <soren.sandm...@gmail.com> > --- > pixman/pixman-image.c | 66 > +-- > 1 file changed, 38 insertions(+), 28 deletions(-) > > diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c > index 1ff1a49..681864e 100644 > --- a/pixman/pixman-image.c > +++ b/pixman/pixman-image.c > @@ -335,37 +335,47 @@ compute_image_info (pixman_image_t *image) > { > flags |= FAST_PATH_NEAREST_FILTER; > } > - else if ( > - /* affine and integer translation components in matrix ... */ > - ((flags & FAST_PATH_AFFINE_TRANSFORM) && > -!pixman_fixed_frac (image->common.transform->matrix[0][2] | > -image->common.transform->matrix[1][2])) && > - ( > - /* ... combined with a simple rotation */ > - (flags & (FAST_PATH_ROTATE_90_TRANSFORM | > - FAST_PATH_ROTATE_180_TRANSFORM | > - FAST_PATH_ROTATE_270_TRANSFORM)) || > - /* ... or combined with a simple non-rotated translation */ > - (image->common.transform->matrix[0][0] == pixman_fixed_1 && > -image->common.transform->matrix[1][1] == pixman_fixed_1 && > -image->common.transform->matrix[0][1] == 0 && > -image->common.transform->matrix[1][0] == 0) > - ) > - ) > + else if (flags & FAST_PATH_AFFINE_TRANSFORM) > { > - /* FIXME: there are some affine-test failures, showing that > -* handling of BILINEAR and NEAREST filter is not quite > -* equivalent when getting close to 32K for the translation > -* components of the matrix. That's likely some bug, but for > -* now just skip BILINEAR->NEAREST optimization in this case. > + /* Suppose the transform is > +* > +*[ t00, t01, t02 ] > +*[ t10, t11, t12 ] > +*[ 0, 0, 1 ] > +* > +* and the destination coordinates are (n + 0.5, m + 0.5). Then > +* the transformed x coordinate is: > +* > +* tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02 > +*= t00 * n + t01 * m + t02 + (t00 + t01) * 0.5 > +* > +* which implies that if t00, t01 and t02 are all integers > +* and (t00 + t01) is odd, then tx will be an integer plus 0.5, > +* which means a BILINEAR filter will reduce to NEAREST. The > same > +* applies in the y direction > */ > - pixman_fixed_t magic_limit = pixman_int_to_fixed (3); > - if (image->common.transform->matrix[0][2] <= magic_limit && > - image->common.transform->matrix[1][2] <= magic_limit && > - image->common.transform->matrix[0][2] >= -magic_limit && > - image->common.transform->matrix[1][2] >= -magic_limit) > + pixman_fixed_t (*t)[3] = image->common.transform->matrix; > + > + if ((pixman_fixed_frac ( > +t[0][0] | t[0][1] | t[0][2] | > +t[1][0] | t[1][1] | t[1][2]) == 0) && > + (pixman_fixed_to_int ( > + (t[0][0] + t[0][1]) & (t[1][0] + t[1][1])) % 2) == 1) > Very clever! This optimization looks correct. > { > - flags |= FAST_PATH_NEAREST_FILTER; > + /* FIXME: there are some affine-test failures, showing that > +* handling of BILINEAR and NEAREST filter is not quite > +* equivalent when getting close to 32K for the translation > +* components of the matrix. That's likely some bug, but > for > +* now just skip BILINEAR->NEAREST optimization in this > case. > +*/ > +
Re: [Pixman] [PATCH v14 08/22] pixman-filter: rename "scale" to "size" when it is 1/scale
On Thu, Mar 17, 2016 at 10:06 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > I suppose it's a little illogical that scale_x and scale_y really are the > reciprocal values of how much the source image should be scaled. I don't > remember exactly what I was thinking, but it might have something like > "this allows you to just pass t[0][0] and t[1][1] if you have a pure > scaling, which avoids a division". There is also kind of precedence in the > Pixman API since the transformation given as in the dst->source direction. > > I don't really like "size" either though. It's not really the size of the > filter that we are specifying; it just happens to be proportional to it. > > If it comes down to "size" and "scale", I prefer "scale". > I tried "width" but it is not actually the filter width. How about "dx" and "dy" since it is almost always the derivative of the x and y in the input (or du,dv or dtx dtx) > > > Søren > > > On Sun, Mar 6, 2016 at 8:06 PM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> This is to remove some confusion when reading the code. "scale" gets >> larger >> as the picture gets larger, while "size" (ie the size of the filter) gets >> smaller. >> >> v14: Removed changes to integral function >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >> --- >> pixman/pixman-filter.c | 18 +- >> pixman/pixman.h| 6 +++--- >> 2 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >> index a29116a..c03a7f6 100644 >> --- a/pixman/pixman-filter.c >> +++ b/pixman/pixman-filter.c >> @@ -221,7 +221,7 @@ static void >> create_1d_filter (int width, >> pixman_kernel_t reconstruct, >> pixman_kernel_t sample, >> - double scale, >> + double size, >> int n_phases, >> pixman_fixed_t *p) >> { >> @@ -251,8 +251,8 @@ create_1d_filter (int width, >> double pos = x + 0.5 - frac; >> double rlow = - filters[reconstruct].width / 2.0; >> double rhigh = rlow + filters[reconstruct].width; >> - double slow = pos - scale * filters[sample].width / 2.0; >> - double shigh = slow + scale * filters[sample].width; >> + double slow = pos - size * filters[sample].width / 2.0; >> + double shigh = slow + size * filters[sample].width; >> double c = 0.0; >> double ilow, ihigh; >> >> @@ -262,7 +262,7 @@ create_1d_filter (int width, >> ihigh = MIN (shigh, rhigh); >> >> c = integral (reconstruct, ilow, >> - sample, 1.0 / scale, ilow - pos, >> + sample, 1.0 / size, ilow - pos, >> ihigh - ilow); >> } >> >> @@ -335,12 +335,12 @@ filter_width (pixman_kernel_t reconstruct, >> pixman_kernel_t sample, double size) >> } >> >> /* Create the parameter list for a SEPARABLE_CONVOLUTION filter >> - * with the given kernels and scale parameters >> + * with the given kernels and size parameters >> */ >> PIXMAN_EXPORT pixman_fixed_t * >> pixman_filter_create_separable_convolution (int *n_values, >> - pixman_fixed_t scale_x, >> - pixman_fixed_t scale_y, >> + pixman_fixed_t size_x, >> + pixman_fixed_t size_y, >> pixman_kernel_t >> reconstruct_x, >> pixman_kernel_t >> reconstruct_y, >> pixman_kernel_t sample_x, >> @@ -348,8 +348,8 @@ pixman_filter_create_separable_convolution (int >>*n_values, >> int >> subsample_bits_x, >> int >> subsample_bits_y) >> { >> -double sx = fabs (pixman_fixed_to_double (scale_x)); >> -double sy = fabs (pixman_fixed_to_double (scale_y)); >> +double sx = fabs (pix
Re: [Pixman] [PATCHv2 2/3] Add new test of filter reduction from BILINEAR to NEAREST
Looks like a good idea to me. It is unfortunate that if it fails there is not much indication which image failed but that would require a lot more checksums. I would guess the checksums are computed by running this with the fast paths disabled? ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 2/2] More general BILINEAR=>NEAREST reduction
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: On Mon, Mar 14, 2016 at 11:19 PM, Søren Sandmann Pedersen < soren.sandm...@gmail.com> wrote: > Generalize and simplify the code that reduces BILINEAR to NEAREST so > that all the reduction happens for all affine transformations where > t00..t22 are integers and (t00 + t01) and (t10 + t11) are both > odd. This is a sufficient condition for the resulting transformed > coordinates being exactly at the center of a pixel so that BILINEAR > becomes identical to NEAREST. > 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. > > Signed-off-by: Søren Sandmann> --- > pixman/pixman-image.c | 69 > ++- > 1 file changed, 41 insertions(+), 28 deletions(-) > > diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c > index 1ff1a49..cff8a30 100644 > --- a/pixman/pixman-image.c > +++ b/pixman/pixman-image.c > @@ -335,37 +335,50 @@ compute_image_info (pixman_image_t *image) > { > flags |= FAST_PATH_NEAREST_FILTER; > } > - else if ( > - /* affine and integer translation components in matrix ... */ > - ((flags & FAST_PATH_AFFINE_TRANSFORM) && > -!pixman_fixed_frac (image->common.transform->matrix[0][2] | > -image->common.transform->matrix[1][2])) && > - ( > - /* ... combined with a simple rotation */ > - (flags & (FAST_PATH_ROTATE_90_TRANSFORM | > - FAST_PATH_ROTATE_180_TRANSFORM | > - FAST_PATH_ROTATE_270_TRANSFORM)) || > - /* ... or combined with a simple non-rotated translation */ > - (image->common.transform->matrix[0][0] == pixman_fixed_1 && > -image->common.transform->matrix[1][1] == pixman_fixed_1 && > -image->common.transform->matrix[0][1] == 0 && > -image->common.transform->matrix[1][0] == 0) > - ) > - ) > + else if (flags & FAST_PATH_AFFINE_TRANSFORM) > { > - /* FIXME: there are some affine-test failures, showing that > -* handling of BILINEAR and NEAREST filter is not quite > -* equivalent when getting close to 32K for the translation > -* components of the matrix. That's likely some bug, but for > -* now just skip BILINEAR->NEAREST optimization in this case. > + /* Suppose the transform is > +* > +*[ t00, t01, t02 ] > +*[ t10, t11, t12 ] > +*[ 120, t21, t22 ] > 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. > +* > +* and the destination coordinates are (n + 0.5, m + 0.5). Then > +* the transformed x coordinate is: > +* > +* tx = t00 * (n + 0.5) + t01 * (m + 0.5) + t02 > +*= t00 * n + t01 * m + t02 + (t00 + t01) * 0.5 > 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. +* which implies that if t00, t01 and t02 are all integers > +* and (t00 + t01) is odd, then tx will be an integer plus 0.5, > +* which means a BILINEAR filter will reduce to NEAREST. The > same > +* applies in the y direction > */ > - pixman_fixed_t magic_limit = pixman_int_to_fixed (3); > - if (image->common.transform->matrix[0][2] <= magic_limit && > - image->common.transform->matrix[1][2] <= magic_limit && > - image->common.transform->matrix[0][2] >= -magic_limit && > - image->common.transform->matrix[1][2] >= -magic_limit) > + pixman_fixed_t (*t)[3] = image->common.transform->matrix; > + > + if (pixman_fixed_frac (t[0][0]) == 0 > && > + pixman_fixed_frac (t[0][1]) == 0 > && > + pixman_fixed_frac (t[0][2]) == 0 > && > + ((t[0][0] + t[0][1]) & pixman_fixed_1) == pixman_fixed_1 > && > + pixman_fixed_frac (t[1][0]) == 0 > && > + pixman_fixed_frac (t[1][1]) == 0 > && > + pixman_fixed_frac (t[1][2]) == 0 > && > + ((t[1][0] + t[1][1]) & pixman_fixed_1) == pixman_fixed_1) > You can or all the numbers together to test they are all integers in one step. > { > - flags |= FAST_PATH_NEAREST_FILTER; > + /* FIXME: there are some
Re: [Pixman] [PATCH 1/2] Add new test of filter reduction from BILINEAR to NEAREST
Great idea to test this. On Mon, Mar 14, 2016 at 11:24 PM, Søren Sandmannwrote: > diff --git a/test/filter-reduction-test.c b/test/filter-reduction-test.c > >> new file mode 100644 >> index 000..72b3142 >> --- /dev/null >> +++ b/test/filter-reduction-test.c >> @@ -0,0 +1,119 @@ >> +/* >> + * Test program, which can detect some problems with nearest neighbour >> + * and bilinear scaling in pixman. Testing is done by running lots >> + * of random SRC and OVER compositing operations a8r8g8b8, x8a8r8g8b8 >> + * and r5g6b5 color formats. >> + * >> + * Script 'fuzzer-find-diff.pl' can be used to narrow down the problem >> in >> + * the case of test failure. >> + */ >> > > This comment is of course a leftover due to starting from a copy of > scaling-test.c. > > > Søren > > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/pixman > > ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 13/14] pixman-image: Implement PIXMAN_FILTER_GOOD/BEST as separable convolutions
On Mon, Mar 7, 2016 at 1:15 AM, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Fri, 4 Mar 2016 19:12:36 -0800 > Bill Spitzak <spit...@gmail.com> wrote: > > > On Fri, Mar 4, 2016 at 4:17 PM, Søren Sandmann <soren.sandm...@gmail.com > > > > wrote: > > > > > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > > > > > >> From: Bill Spitzak <spit...@gmail.com> > > >> > > >> Detects and uses PIXMAN_FILTER_NEAREST for all 8 90-degree rotations > and > > >> reflections when the scale is 1.0 and integer translation. > > >> > > >> GOOD uses: > > >> > > >> scale < 1/16 : BOX.BOX at size 16 > > >> scale < 3/4 : BOX.BOX at size 1/scale > > >> larger : BOX.BOX at size 1 > > >> > > >> If both directions have a scale >= 3/4 or a scale of 1/2 and an > integer > > >> translation, the faster PIXMAN_FILTER_BILINEAR code is used. This is > > >> compatable at these scales with older versions of pixman where > bilinear > > >> was always used for GOOD. > > >> > > >> BEST uses: > > >> > > >> scale < 1/24 : BOX.BOX at size 24 > > >> scale < 1/16 : BOX.BOX at size 1/scale > > >> scale < 1 : IMPULSE.LANCZOS2 at size 1/scale > > >> scale < 2.333 : IMPULSE.LANCZOS2 at size 1 > > >> scale < 128 : BOX.LANCZOS2 at size 1/(scale-1) (antialiased square > > >> pixels) > > >> larger : BOX.LANCZOS2 at size 1/127 (antialias blur gets thicker) > > >> > > >> v8: Cutoff in BEST between IMPULSE.LANCZOS2 and BOX.LANCZOS2 adjusted > for > > >> a better match between the filters. > > >> > > >> v9: Use the new negative subsample controls to scale the subsamples. > These > > >> were chosen by finding the lowest number that did not add visible > > >> artifacts to the zone plate image. > > >> > > >> Scale demo altered to default to GOOD and locked-together x+y > scale > > >> > > >> Fixed divide-by-zero from all-zero matrix found by stress-test > > >> > > >> v11: Whitespace and formatting fixes > > >> Moved demo changes to a later patch > > >> > > >> v12: Whitespace and formatting fixes > > >> > > >> Signed-off-by: Bill Spitzak <spit...@gmail.com> > > >> > > > > > > The short answer to this patch is NACK - it doesn't make sense to > change > > > the meaning of GOOD and BEST. > > > > > > > Okay that is going to need a big WTF? > > No, it's perfectly understandable. > > > > There are at least three reasons: > > > > > > 1. Pixman is a low-level API that is in the camp of "do what I say" not > > > "do what I mean". When users such as cairo specify GOOD and BEST, they > are > > > essentially asking Pixman to make a tradeoff that it doesn't have the > > > knowledge to do. As such, I think it was a mistake to have these > values in > > > the first place (and indeed also a mistake to have them in the Render > > > extension since X11 is also supposed to be a do-what-I-say API). As > such, I > > > think they should remain aliases to BILINEAR as they are now. > > > > > > > I have no idea why having three ways to say BILINEAR is necessary to "do > > what I say". One seems sufficient. > > You must forget any resemblance the strings "GOOD" and "BEST" have > with English, and see them as abstract names for particular > mathematical functions. > > That is what do-what-I-say means. > > Therefore you cannot change what mathematical function they correspond > to. > > There are three ways of saying the same only because of a mistake in > the past which became evident only much later. It happens. Get over it. > > > > The way to go is to add a new SeparateConvolution filter to Render and > make > > > fb call into Pixman to implement it. Once that is in place, the > hardware > > > drivers can then implement it. > > > > > > > You seem to think the hardware drivers will implement this version of > > SeparateConvolution? Really??? > > That is implied by do-what-I-say API policy. > > > The purpose of this was so that the hardware drivers could implement > > something they call "good" and something they call "best" using wha
Re: [Pixman] [PATCH v13 13/14] pixman-image: Implement PIXMAN_FILTER_GOOD/BEST as separable convolutions
I believe on the good/best patch you are worried about the freeing of the filter array. However in commit 756b54f6 you made pixman_set_filter always copy the one provided by the caller. This means my code works because the filter cannot be changed away from/to good/best without also freeing the previous data. The commit just says "Add boolean returns to various setters" so I am wondering if this is a mistake. There is also a comparison of the old/new pointers which won't work if the filter array is copied. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 14/14] demos/scale: default to GOOD and locked-together axis
On Fri, Mar 4, 2016 at 4:19 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> >> >> This makes the demo match normal behavior of pixman/cairo at startup. >> > > Defaulting to 'locked' makes sense, but in the light of the comments to > the earlier patches, defaulting to GOOD doesn't. > Since GOOD is the default for Cairo/Pixman, I think it does make sense. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 13/14] pixman-image: Implement PIXMAN_FILTER_GOOD/BEST as separable convolutions
On Fri, Mar 4, 2016 at 4:17 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> Detects and uses PIXMAN_FILTER_NEAREST for all 8 90-degree rotations and >> reflections when the scale is 1.0 and integer translation. >> >> GOOD uses: >> >> scale < 1/16 : BOX.BOX at size 16 >> scale < 3/4 : BOX.BOX at size 1/scale >> larger : BOX.BOX at size 1 >> >> If both directions have a scale >= 3/4 or a scale of 1/2 and an integer >> translation, the faster PIXMAN_FILTER_BILINEAR code is used. This is >> compatable at these scales with older versions of pixman where bilinear >> was always used for GOOD. >> >> BEST uses: >> >> scale < 1/24 : BOX.BOX at size 24 >> scale < 1/16 : BOX.BOX at size 1/scale >> scale < 1 : IMPULSE.LANCZOS2 at size 1/scale >> scale < 2.333 : IMPULSE.LANCZOS2 at size 1 >> scale < 128 : BOX.LANCZOS2 at size 1/(scale-1) (antialiased square >> pixels) >> larger : BOX.LANCZOS2 at size 1/127 (antialias blur gets thicker) >> >> v8: Cutoff in BEST between IMPULSE.LANCZOS2 and BOX.LANCZOS2 adjusted for >> a better match between the filters. >> >> v9: Use the new negative subsample controls to scale the subsamples. These >> were chosen by finding the lowest number that did not add visible >> artifacts to the zone plate image. >> >> Scale demo altered to default to GOOD and locked-together x+y scale >> >> Fixed divide-by-zero from all-zero matrix found by stress-test >> >> v11: Whitespace and formatting fixes >> Moved demo changes to a later patch >> >> v12: Whitespace and formatting fixes >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> > > The short answer to this patch is NACK - it doesn't make sense to change > the meaning of GOOD and BEST. > Okay that is going to need a big WTF? There are at least three reasons: > > 1. Pixman is a low-level API that is in the camp of "do what I say" not > "do what I mean". When users such as cairo specify GOOD and BEST, they are > essentially asking Pixman to make a tradeoff that it doesn't have the > knowledge to do. As such, I think it was a mistake to have these values in > the first place (and indeed also a mistake to have them in the Render > extension since X11 is also supposed to be a do-what-I-say API). As such, I > think they should remain aliases to BILINEAR as they are now. > I have no idea why having three ways to say BILINEAR is necessary to "do what I say". One seems sufficient. 2. The motivation for doing this in Pixman (and not in cairo) is that > supposedly the X server will then pick it up automatically and use the > high-quality filters when PictFilterBest and PictFilterGood is set. But > that doesn't actually work because the X server doesn't map those Render > filters to the corresponding Pixman filters. See: > > https://cgit.freedesktop.org/xorg/xserver/tree/fb/fbpict.c#n420 > > So it looks to me like the whole scheme just won't work. But even if it > did, you would still need to make all the hardware drivers do the same > thing, so it's not just a matter of adding some constants in that function. > If X is not currently sending GOOD/BEST through, then this is even a better reason why it is ok to change them, as it cannot possibly change the behavior of any X programs. All other pixman-using programs are directly linked with the library. I did figure X would remain broken for long after this is fixed, and Cairo would use the image backend for such transforms until X is updated, or it is replaced with Wayland (where the image backend is used all the time). The way to go is to add a new SeparateConvolution filter to Render and make > fb call into Pixman to implement it. Once that is in place, the hardware > drivers can then implement it. > You seem to think the hardware drivers will implement this version of SeparateConvolution? Really??? The purpose of this was so that the hardware drivers could implement something they call "good" and something they call "best" using whatever hardware resources are available. This is not going to happen if they are expected to read the filter array. 3. The patch is completely broken as written. I could go into details, but > there is no point since I don't think the patch should be pushed even if > fixed. I'll just point out that if the environment variable > PIXMAN_DISABLE=fast is set, the patch does absolutely nothing (you can > verify this with the scale program). And setting this envi
Re: [Pixman] [PATCH v13 11/14] pixman-filter: Negative subsample values produces scaled result
On Fri, Mar 4, 2016 at 3:49 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Tue, Feb 16, 2016 at 4:45 PM, Bill Spitzak <spit...@gmail.com> wrote: > >> I have a new version of this patch, which fixes a math error that >> caused it to produce too many samples at small sizes (large scales). I >> am not clear if I can submit a v14 of just this patch or I should >> instead submit a v14 of the entire patch series. >> > > I don't think this functionality belongs in pixman at all. Computing the > number of subsamples is essentially a performance/quality tradeoff, which > is something that cairo and other clients need to make for themselves. In > general, pixman is not the place for heuristics; its API should be > predictable and explicit. > > So, NACK from me on this patch. > The scale demo is almost useless without this as you have to adjust this every time you change the scale. Most software I am familiar with use a fixed number of samples for the width of the filter, not per integer. This was an attempt to simulate that. I guess the scale demo could do this math, though that makes it rather difficult to use the scale demo to figure out the best settings. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 10/14] pixman-filter: Gaussian fixes
On Fri, Mar 4, 2016 at 3:39 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > > > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> The SIGMA term drops out on simplification. >> > > But SIGMA has a meaning here: it's the standard deviation for the normal > distribution corresponding to the Gaussian in question, so unless there is > some good argument for reducing the formula, I think it should stay the way > it is. > The simplification does not happen due to inexact math. > > Expanding the size is fine with me > This is based on checking other software for the widths it uses for such filters. There is a bit of hf at the cutoffs, enlarging the size reduces this. It is very common for "gaussian" to actually be this function multiplied by a sinc window to avoid this, but I did not think that change was a good idea. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 08/14] pixman-filter: Corrections to the integral() function
On Fri, Mar 4, 2016 at 3:12 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@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 fixed the nice filter and the integration to directly produce normalized >> values. Re-normalization is still needed for impulse.box or >> impulse.triangle >> so I did not remove it. >> >> Distribute fixed error over all filter samples, to remove a high-frequency >> bit of noise in the center of some filters (lancoz at large scale value). >> >> box.box, which I expect will be very common as it is the proposed "good" >> filter, >> was made a lot faster and more accurate. This is easy as the caller >> already >> intersected the two boxes, so the width is the integral. >> >> v7: This is a merge of 4 patches and lots of new code cleanup and fixes >> determined by examining the gnuplot output >> > > Please split this back out into separate patches that each fix one problem: > > - Fixing IMPULSE special cases > - Distributing errors evenly > - BOX.BOX speedups > > There are several of these changes I'm not convinced about, and they are > hard to review as one big patch. > I will try to split them up. This is the unfortunate result of trying to rebase them together, I merged because of conflicts. As you don't like the argument renaming I might as well fix all of these at the same time. diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > >> index 8b8fb82..e82871f 100644 >> --- a/pixman/pixman-filter.c >> +++ b/pixman/pixman-filter.c >> @@ -79,7 +79,7 @@ sinc (double x) >> } >> >> static double >> -lanczos (double x, int n) >> +lanczos (double x, double n) >> { >> return sinc (x) * sinc (x * (1.0 / n)); >> } >> > > What is the point of this change? I don't think Lanczos makes sense with a > non-integral number of lobes. > It removes the int->float cast, but I think this was mostly accidental. Inline would probably remove the cast as well, and the division too. @@ -99,7 +99,7 @@ lanczos3_kernel (double x) >> static double >> nice_kernel (double x) >> { >> -return lanczos3_kernel (x * 0.75); >> +return lanczos3_kernel (x * 0.75) * 0.75; >> } >> > > I don't think this makes sense given that normalization is happening later > anyway. > It is the only non-normalized one, and fixing this was very useful for finding out whether bugs were in the normalization or in the filter generators. Also it is plausable that future ones may be able to remove the normalization, though I could not because of the box and triangle being so non-continuous that they needed it. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 09/14] pixman-filter: Nested polynomial for cubic
On Fri, Mar 4, 2016 at 3:05 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> v11: Restored range checks >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >> > > What's the point of this? All this does is putting ax^2 outside of the > parenthesis? > Nested form reduces the number of multiplications (from 23 to 18 in this case), without changing how many additions are done. And compilers can take advantage of ax+b instructions with this (I did not check if that actually happened, however). ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 07/14] pixman-filter: Correct Simpsons integration
On Fri, Mar 4, 2016 at 3:00 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> Simpsons uses cubic curve fitting, with 3 samples defining each cubic. >> This >> makes the weights of the samples be in a pattern of 1,4,2,4,2...4,1, and >> then >> dividing the result by 3. >> >> The previous code was using weights of 1,2,6,6...6,2,1. Since it divided >> by >> 3 this produced about 2x the desired value (the normalization fixed this). >> Also this is effectively a linear interpolation, not Simpsons integration. >> > > It is not true that the previous code used these weights because it only > ran for half the segments (it had += 2). The intention with the code was to > do both the 4 and the 2 weight in one loop, but for that to work a1 and a2 > would have to be updated, which the code didn't do. So it was indeed buggy, > and I think the new code is correct. > You are right, the old code did weights of 1,2,0,6,0,6,...,2,1. This would explain why it worked even when I removed normalization. There should be spaces before the parentheses in the SAMPLE() macros, > though. > Ok will fix. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 06/14] pixman-filter: reduce amount of malloc/free/memcpy to generate filter
On Fri, Mar 4, 2016 at 2:52 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > > > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> Rearranged so that the entire block of memory for the filter pair >> is allocated first, and then filled in. Previous version allocated >> and freed two temporary buffers for each filter and did an extra >> memcpy. >> >> v8: small refactor to remove the filter_width function >> >> v10: Restored filter_width function but with arguments changed to >> match later patches >> >> v11: Removed unused arg and pointer from filter_width function >> Whitespace fixes. >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >> > > I'm not opposed to this patch if it is correct (and I'll believe the > existing Reviewed-by for correctness), but I'm also don't think there is > much point to this -- these malloc() and free() calls are unlikely to be > performance bottlenecks. > > Acked-by: Soren Sandmann <soren.sandm...@gmail.com> > This is certainly correct, it would have been obvious in my tests if it did not work. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 05/14] pixman-filter: Consistency in arg names to integral ()
On Fri, Mar 4, 2016 at 2:51 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> Rename kernel1/2 to reconstruct/sample to match the other functions. >> Rename "scale" to "size" to avoid confusion with the scale being applied >> to the image, which is the reciprocol of this value. >> >> v10: Renamed "scale" to "size" >> > > I don't really agree with this patch. The intention behind the code was > that the integral() function shouldn't know anything about scaling or > images; all it would do was compute integrals over functions, but it > wouldn't know or care what those functions were used for. > Okay will keep the arguments as before. In fact "scale" is used as expected in the old version of the code (larger numbers mean the picture is being enlarged). I still think renaming kernel1/2 to be the same name used in other functions makes things a lot easier to follow. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 03/14] demos/scale: Only generate filters when used for separable
On Fri, Mar 4, 2016 at 2:44 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > > > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> This makes the speed of the demo more accurate, as the filter generation >> is a visible fraction of the time it takes to do a transform. This also >> prevents the output of unused filters in the gnuplot option in the next >> patch. >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >> > > This depends on patch 2, which I don't think should be accepted. However, > if patch 2 is pushed, this one is an improvement that should also be > pushed. It would make sense to squash these two patches and also include > the gray-out functionality in that squashed patch. > Even without patch 2 it could be set to bilinear, so this actually is useful. You are probably right that it does not apply cleanly without 2. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 02/14] demos/scale: Added pulldown to choose PIXMAN_FILTER_* value
On Fri, Mar 4, 2016 at 2:41 PM, Søren Sandmann <soren.sandm...@gmail.com> wrote: > On Wed, Feb 10, 2016 at 1:25 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> >> >> This allows testing of GOOD/BEST and to do comparisons between >> the basic filters and PIXMAN_FILTER_SEPARABLE_CONVOLUTION settings. >> >> Signed-off-by: Bill Spitzak <spit...@gmail.com> >> Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> >> > > > Given that I don't think the GOOD/BEST/FAST filters should be changed (see > upcoming comments to patch 13), there is not much point to this patch. > However, if it ends up going in, the non-filter settings should certainly > be grayed out when the filter is not SEPARABLE since they have no effect in > that case. (Also, of course, if the GOOD/BEST/FAST filters stay as simple > aliases, as they should, only NEAREST and LINEAR make sense in the dropdown > here). > The ability to adjust the settings for separable while it was set to other filters was very useful, actually. Is there a way in GTK to gray out a widget while not making it not work? If not I guess disabling them will work. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v13 11/14] pixman-filter: Negative subsample values produces scaled result
On 02/16/2016 01:58 PM, Oded Gabbay wrote: On Tue, Feb 16, 2016 at 11:45 PM, Bill Spitzak <spit...@gmail.com> wrote: I have a new version of this patch, which fixes a math error that caused it to produce too many samples at small sizes (large scales). I am not clear if I can submit a v14 of just this patch or I should instead submit a v14 of the entire patch series. ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman My suggestion is to wait for Soren's review (I believe we will see something around the start of next week) and when you fix his comments, add this fix as well and then mark it as v14. Oded Have you been able to contact Søren? ___ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v10 15/15] pixman-image: Implement PIXMAN_FILTER_GOOD/BEST as separable convolutions
On 02/04/2016 05:42 AM, Oded Gabbay wrote: On Tue, Feb 2, 2016 at 8:28 AM, <spit...@gmail.com> wrote: From: Bill Spitzak <spit...@gmail.com> Detects and uses PIXMAN_FILTER_NEAREST for all 8 90-degree rotations and reflections when the scale is 1.0 and integer translation. GOOD uses: scale < 1/16 : BOX.BOX at size 16 scale < 3/4 : BOX.BOX at size 1/scale larger : BOX.BOX at size 1 If both directions have a scale >= 3/4 or a scale of 1/2 and an integer translation, the faster PIXMAN_FILTER_BILINEAR code is used. This is compatable at these scales with older versions of pixman where bilinear was always used for GOOD. BEST uses: scale < 1/24 : BOX.BOX at size 24 scale < 1/16 : BOX.BOX at size 1/scale scale < 1 : IMPULSE.LANCZOS2 at size 1/scale scale < 2.333 : IMPULSE.LANCZOS2 at size 1 scale < 128 : BOX.LANCZOS2 at size 1/(scale-1) (antialiased square pixels) larger : BOX.LANCZOS2 at size 1/127 (antialias blur gets thicker) v8: Cutoff in BEST between IMPULSE.LANCZOS2 and BOX.LANCZOS2 adjusted for a better match between the filters. v9: Use the new negative subsample controls to scale the subsamples. These were chosen by finding the lowest number that did not add visible artifacts to the zone plate image. Scale demo altered to default to GOOD and locked-together x+y scale Let's separate pixman core changes and demo changes. It's bad for bisectability and maintainability. Do the core changes first, then another patch for the scale demo. Fixed divide-by-zero from all-zero matrix found by stress-test Signed-off-by: Bill Spitzak <spit...@gmail.com> --- demos/scale.c | 10 +- demos/scale.ui| 1 + pixman/pixman-image.c | 289 -- 3 files changed, 216 insertions(+), 84 deletions(-) diff --git a/demos/scale.c b/demos/scale.c index 881004e..3df4442 100644 --- a/demos/scale.c +++ b/demos/scale.c @@ -340,7 +340,7 @@ on_expose (GtkWidget *da, GdkEvent *event, gpointer data) static void set_up_combo_box (app_t *app, const char *box_name, - int n_entries, const named_int_t table[]) + int n_entries, const named_int_t table[], int active) { GtkWidget *widget = get_widget (app, box_name); GtkListStore *model; @@ -366,7 +366,7 @@ set_up_combo_box (app_t *app, const char *box_name, gtk_list_store_set (model, , 0, info->name, -1); } -gtk_combo_box_set_active (GTK_COMBO_BOX (widget), 0); +gtk_combo_box_set_active (GTK_COMBO_BOX (widget), active); g_signal_connect (widget, "changed", G_CALLBACK (rescale), app); } @@ -374,7 +374,7 @@ set_up_combo_box (app_t *app, const char *box_name, static void set_up_filter_box (app_t *app, const char *box_name) { -set_up_combo_box (app, box_name, G_N_ELEMENTS (filters), filters); +set_up_combo_box (app, box_name, G_N_ELEMENTS (filters), filters, 0); } static char * @@ -422,14 +422,14 @@ app_new (pixman_image_t *original) widget = get_widget (app, "drawing_area"); g_signal_connect (widget, "expose_event", G_CALLBACK (on_expose), app); -set_up_combo_box (app, "filter_combo_box", G_N_ELEMENTS (filter_types), filter_types); +set_up_combo_box (app, "filter_combo_box", G_N_ELEMENTS (filter_types), filter_types, 3); set_up_filter_box (app, "reconstruct_x_combo_box"); set_up_filter_box (app, "reconstruct_y_combo_box"); set_up_filter_box (app, "sample_x_combo_box"); set_up_filter_box (app, "sample_y_combo_box"); set_up_combo_box ( -app, "repeat_combo_box", G_N_ELEMENTS (repeats), repeats); + app, "repeat_combo_box", G_N_ELEMENTS (repeats), repeats, 0); g_signal_connect ( gtk_builder_get_object (app->builder, "lock_checkbutton"), diff --git a/demos/scale.ui b/demos/scale.ui index 1e77f56..13e0e0d 100644 --- a/demos/scale.ui +++ b/demos/scale.ui @@ -177,6 +177,7 @@ id="lock_checkbutton"> Lock X and Y Dimensions 0.0 + True False diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c index 1ff1a49..c381260 100644 --- a/pixman/pixman-image.c +++ b/pixman/pixman-image.c @@ -28,6 +28,7 @@ #include #include #include +#include #include "pixman-private.h" @@ -274,112 +275,242 @@ compute_image_info (pixman_image_t *image) FAST_PATH_X_UNIT_POSITIVE | FAST_PATH_Y_UNIT_ZERO | FAST_PATH_AFFINE_TRANSFORM); + switch (image->common.filter) + { + case PIXMAN_FILTER_CONVOLUTION: + break; + case PIXMAN_FILTER_SEPARABLE_CONVOLUTION: +
Re: [Pixman] [PATCH v9 06/15] pixman-filter: Correct Simpsons integration
DOH! You are right, I should make sure each step compiles. I am also very much tempted to change "scale" to "size" to avoid the fact that the value is approximately the reciprocal of what must users would call the "scale" of the transform. That will require testing each patch. On Mon, Feb 1, 2016 at 6:34 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Fri, Jan 22, 2016 at 11:42 AM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > Simpsons uses cubic curve fitting, with 3 samples defining each cubic. > This > > makes the weights of the samples be in a pattern of 1,4,2,4,2...4,1, and > then > > dividing the result by 3. > > > > The previous code was using weights of 1,2,6,6...6,2,1. Since it divided > by > > 3 this produced about 2x the desired value (the normalization fixed > this). > > Also this is effectively a linear interpolation, not Simpsons > integration. > > > > With this fix the integration is accurate enough that the number of > samples > > could be reduced a lot. Multiples of 12 seem to work best. > > > > v9: Changed samples from 16 to 12 > > v7: Merged with patch to reduce from 128 samples to 16 > > > > Signed-off-by: Bill Spitzak <spit...@gmail.com> > > --- > > pixman/pixman-filter.c | 29 +++-- > > 1 file changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 55073c4..718649a 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -189,13 +189,19 @@ integral (pixman_kernel_t reconstruct, double x1, > > } > > else > > { > > - /* Integration via Simpson's rule */ > > -#define N_SEGMENTS 128 > > -#define SAMPLE(a1, a2) \ > > - (filters[reconstruct].func ((a1)) * filters[sample].func ((a2) / > scale)) > > - > > + /* Integration via Simpson's rule > > +* See http://www.intmath.com/integration/6-simpsons-rule.php > > +* 12 segments (6 cubic approximations) seems to produce best > > +* result for lanczos3.linear, which was the combination that > > +* showed the most errors. This makes sense as the lanczos3 > > +* filter is 6 wide. > > +*/ > > +#define N_SEGMENTS 12 > > +#define SAMPLE(a) \ > > + (filters[reconstruct].func ((a)) * filters[sample].func (((a) - > pos) / scale)) > > + > You changed the SAMPLE macro to get 1 parameter, but in all the calls > you send 2 parameters, which make the compilation fail. > > Please fix this and resend the patch. > > Oded > > > double s = 0.0; > > - double h = width / (double)N_SEGMENTS; > > + double h = width / N_SEGMENTS; > > int i; > > > > s = SAMPLE (x1, x2); > > @@ -204,11 +210,14 @@ integral (pixman_kernel_t reconstruct, double x1, > > { > > double a1 = x1 + h * i; > > double a2 = x2 + h * i; > > + s += 4 * SAMPLE(a1, a2); > > + } > > > > - s += 2 * SAMPLE (a1, a2); > > - > > - if (i >= 2 && i < N_SEGMENTS - 1) > > - s += 4 * SAMPLE (a1, a2); > > + for (i = 2; i < N_SEGMENTS; i += 2) > > + { > > + double a1 = x1 + h * i; > > + double a2 = x2 + h * i; > > + s += 2 * SAMPLE(a1, a2); > > } > > > > s += SAMPLE (x1 + width, x2 + width); > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v9 03/15] pixman-image: Added enable-gnuplot config to view filters in gnuplot
On Mon, Feb 1, 2016 at 6:11 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Mon, Feb 1, 2016 at 4:10 PM, Oded Gabbay <oded.gab...@gmail.com> wrote: > > On Fri, Jan 22, 2016 at 11:42 AM, <spit...@gmail.com> wrote: > >> From: Bill Spitzak <spit...@gmail.com> > >> > >> If enable-gnuplot is configured, then you can pipe the output of a > pixman-using program > >> to gnuplot and get a continuously-updated plot of the horizontal > filter. This > >> works well with demos/scale to test the filter generation. > >> > >> The plot is all the different subposition filters shuffled together. > This is > >> misleading in a few cases: > >> > >> IMPULSE.BOX - goes up and down as the subfilters have different > numbers of non-zero samples > >> IMPULSE.TRIANGLE - somewhat crooked for the same reason > >> 1-wide filters - looks triangular, but a 1-wide box would be more > accurate > >> > >> v8: Use config option > >> Moved code to the filter generator > >> Modified scale demo to not call filter generator a second time. > >> > >> v7: First time this ability was included > > One more thing. The vX comments should be ordered from first to last > (old to new), so: > v7: ... > v8: ... > Okay thank you, I will fix all of them. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v9 03/15] pixman-image: Added enable-gnuplot config to view filters in gnuplot
On Mon, Feb 1, 2016 at 6:10 AM, Oded Gabbaywrote: > > > -params = pixman_filter_create_separable_convolution ( > > -_params, > > -sx * 65536.0 + 0.5, > > - sy * 65536.0 + 0.5, > > - get_value (app, filters, "reconstruct_x_combo_box"), > > - get_value (app, filters, "reconstruct_y_combo_box"), > > - get_value (app, filters, "sample_x_combo_box"), > > - get_value (app, filters, "sample_y_combo_box"), > > - gtk_adjustment_get_value (app->subsample_adjustment), > > - gtk_adjustment_get_value (app->subsample_adjustment)); > > +if (get_value (app, filter_types, "filter_combo_box") == > > + PIXMAN_FILTER_SEPARABLE_CONVOLUTION) > > +{ > > + params = pixman_filter_create_separable_convolution ( > > +_params, > > + sx * 65536.0 + 0.5, > > + sy * 65536.0 + 0.5, > > + get_value (app, filters, "reconstruct_x_combo_box"), > > + get_value (app, filters, "reconstruct_y_combo_box"), > > + get_value (app, filters, "sample_x_combo_box"), > > + get_value (app, filters, "sample_y_combo_box"), > > + gtk_adjustment_get_value (app->subsample_adjustment), > > + gtk_adjustment_get_value (app->subsample_adjustment)); > > +} > > +else > > +{ > > + params = 0; > > + n_params = 0; > > +} > > Wait, what the above code has to do with this patch ? > It wasn't in the previous version (v7) and I don't see how it is > related to the gnuplot. > This seems like a fix to the demo code. > If what I said is correct, please split it into a different patch. > I had to patch it so it did not run the filter generator twice when set to good/best, otherwise the plot flashed back and forth annoyingly. But this also makes the demo a bit faster showing accurately the speed at which transforms are done, so I think putting this in an earlier patch is a good idea. > +#if PIXMAN_GNUPLOT > > To keep consistency with other defines checks, please use #ifdef when > checking just one define > OK > > @@ -346,5 +387,9 @@ out: > > free (horz); > > free (vert); > > > > +#if PIXMAN_GNUPLOT > > To keep consistency with other defines checks, please use #ifdef when > checking just one define > > > +gnuplot_filter(width, subsample_x, params+4); > > +#endif > > + > > What's the point in printing the filter after the out: label ? > You can get here in cases where there were errors in the function. > Why not put the call to gnuplot_filter() just before the out: label ? > You are correct it should only do this on success. My mistake. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH v9 01/15] demos/scale: Compute filter size using boundary of xformed ellipse, not rectangle
Okay, thanks for the info. Will try to fix them all. On Mon, Feb 1, 2016 at 5:51 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Fri, Jan 22, 2016 at 11:41 AM, <spit...@gmail.com> wrote: > > > > From: Bill Spitzak <spit...@gmail.com> > > > > This is much more accurate and less blurry. In particular the filtering > does > > not change as the image is rotated. > > > > Signed-off-by: Bill Spitzak <spit...@gmail.com> > > --- > > demos/scale.c | 102 > +++--- > > 1 file changed, 61 insertions(+), 41 deletions(-) > > > > diff --git a/demos/scale.c b/demos/scale.c > > index d00307e..0995ad0 100644 > > --- a/demos/scale.c > > +++ b/demos/scale.c > > @@ -55,50 +55,70 @@ get_widget (app_t *app, const char *name) > > return widget; > > } > > > > -static double > > -min4 (double a, double b, double c, double d) > > -{ > > -double m1, m2; > > - > > -m1 = MIN (a, b); > > -m2 = MIN (c, d); > > -return MIN (m1, m2); > > -} > > - > > -static double > > -max4 (double a, double b, double c, double d) > > -{ > > -double m1, m2; > > - > > -m1 = MAX (a, b); > > -m2 = MAX (c, d); > > -return MAX (m1, m2); > > -} > > - > > +/* Figure out the boundary of a diameter=1 circle transformed into an > ellipse > > + * by trans. Proof that this is the correct calculation: > > + * > > + * Transform x,y to u,v by this matrix calculation: > > + * > > + * |u| |a c| |x| > > + * |v| = |b d|*|y| > > + * > > + * Horizontal component: > > + * > > + * u = ax+cy (1) > > + * > > + * For each x,y on a radius-1 circle (p is angle to the point): > > + * > > + * x^2+y^2 = 1 > > + * x = cos(p) > > + * y = sin(p) > > + * dx/dp = -sin(p) = -y > > + * dy/dp = cos(p) = x > > + * > > + * Figure out derivative of (1) relative to p: > > + * > > + * du/dp = a(dx/dp) + c(dy/dp) > > + *= -ay + cx > > + * > > + * The min and max u are when du/dp is zero: > > + * > > + * -ay + cx = 0 > > + * cx = ay > > + * c = ay/x (2) > > + * y = cx/a (3) > > + * > > + * Substitute (2) into (1) and simplify: > > + * > > + * u = ax + ay^2/x > > + *= a(x^2+y^2)/x > > + *= a/x (because x^2+y^2 = 1) > > + * x = a/u (4) > > + * > > + * Substitute (4) into (3) and simplify: > > + * > > + * y = c(a/u)/a > > + * y = c/u (5) > > + * > > + * Square (4) and (5) and add: > > + * > > + * x^2+y^2 = (a^2+c^2)/u^2 > > + * > > + * But x^2+y^2 is 1: > > + * > > + * 1 = (a^2+c^2)/u^2 > > + * u^2 = a^2+c^2 > > + * u = hypot(a,c) > > + * > > + * Similarily the max/min of v is at: > > + * > > + * v = hypot(b,d) > > + * > > + */ > > static void > > compute_extents (pixman_f_transform_t *trans, double *sx, double *sy) > > { > > -double min_x, max_x, min_y, max_y; > > -pixman_f_vector_t v[4] = > > -{ > > - { { 1, 1, 1 } }, > > - { { -1, 1, 1 } }, > > - { { -1, -1, 1 } }, > > - { { 1, -1, 1 } }, > > -}; > > - > > -pixman_f_transform_point (trans, [0]); > > -pixman_f_transform_point (trans, [1]); > > -pixman_f_transform_point (trans, [2]); > > -pixman_f_transform_point (trans, [3]); > > - > > -min_x = min4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); > > -max_x = max4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); > > -min_y = min4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); > > -max_y = max4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); > > - > > -*sx = (max_x - min_x) / 2.0; > > -*sy = (max_y - min_y) / 2.0; > > +*sx = hypot (trans->m[0][0], trans->m[0][1]) / trans->m[2][2]; > > +*sy = hypot (trans->m[1][0], trans->m[1][1]) / trans->m[2][2]; > > } > > > > typedef struct > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> > > p.s. if we have additional versions of this patch series, and patches > that got r-b are not modified, then please add my r-b to the patch so > I would know I don't need to spend even a second over that patch. > > Thanks > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] configure: add options to disable demos and tests
The ability to disabling everything except the library itself seems like a good idea. Make sure "make test" fails if tests are disabled. I do think it would be nice to actually patch the tests so they compile on these platforms. On Wed, Jan 20, 2016 at 1:13 AM, Thomas Petazzoni < thomas.petazz...@free-electrons.com> wrote: > Hello, > > On Wed, 20 Jan 2016 09:27:46 +0200, Siarhei Siamashka wrote: > > > Thanks for this patch. Though if building (and using) pixman on > > such platforms is wanted, then a much better solution would be to > > update the problematic tests and make them compile. Skipping some > > sub-tests is better than having no tests at all. I also remember > > your patch for FE_DIVBYZERO from a few months ago: > > > > > http://lists.freedesktop.org/archives/pixman/2015-September/004019.html > > > > Is it still the same Microblaze or Nios2 architecture that is causing > > problems for you? > > Yes, it is. Other architectures might be affected because the > implementation in uClibc is not complete for all architectures. > > > While adding new configure options just adds functionality and > > preserves the existing behavior, I don't feel very happy about > > the fact that this provides an easy way to ignore problems instead > > of fixing them. It would be really great is somebody tried to run > > the pixman test suite ("make check") on these architectures at > > least once. > > > > Encountering compiler bugs is unfortunately a regular occurrence > > for pixman. For example, not so long ago, GCC 4.9 miscompiled > > pixman on ARM (fortunately, the broken code was in the test suite > > itself and not in the pixman library): > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64172 > > > > And even just a few days ago, pixman was one of the victims during > > a GCC 6 snapshot test (an easy to notice ICE during a distro test > > rebuild): > > > > https://gcc.gnu.org/ml/gcc/2016-01/msg00101.html > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66856 > > > > What I'm trying to say is that there had been many compiler bugs > > affecting pixman during the last few years. Now you are dealing > > with uncommon architectures, and the compilers there are probably > > even less mature than GCC on x86 / arm / powerpc. > > I agree, but those options also allow to skip building things that > won't be used, even if they actually build properly. On ARM, x86, > PowerPC and other "mainstream" architectures, the demos and tests build > fine, but they are not used at all by Buildroot, so it's just spending > time building things that aren't necessary. > > So even if those tests and demos were building for all architectures, > it would still be useful to have a way to *not* build them. > > But I'll have a look at re-enabling the building of the pixman tests in > Buildroot. Now that the FE_* things are disabled in the tests when not > available (after commit 4297e9058d252cac653723fe0b1bee559fbac3a4). > > Thanks! > > Thomas > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter
Yes I will try to send new versions asap. Known mistakes/problems: 1. I was a bit too fast at deleting the recursion code for the linear filters. The problems were not very visible with the 16-segment simpsons integration, but obvious when reducing to 4 (which otherwise works in most cases). Also it is actually easier to split with my new arguments to the function, so I really should not have done that. 2. x.LINEAR where x is not IMPULSE or BOX or LINEAR is producing artefacts that don't seem to exist in other combinations (or are much less visible in other combinations?). I think I need to investigate this. 3. My comment is somewhat inaccurate and I need to update it. LINEAR.LINEAR is only a cubic at scale==1, it is not possible to replicate what other software calls "cubic" or "bicubic" using this On Mon, Jan 11, 2016 at 1:16 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Fri, Jan 8, 2016 at 8:53 PM, Bill Spitzak <spit...@gmail.com> wrote: > > > > > > On Thu, Jan 7, 2016 at 11:44 PM, Pekka Paalanen <ppaala...@gmail.com> > wrote: > >> > >> > >> Please keep in mind that the filters GOOD and BEST have been as is for > >> a long long time, AFAIU, so changing their behaviour now is likely not > >> a good idea. They are no longer "a good filter" and "whatever best > >> filter", but "the specific filter called GOOD" and "the specific filter > >> called BEST", in lack of documentation saying otherwise. > > > > > > Both GOOD and BEST are identical to BILINEAR in the current version of > > Pixman. Therefore anybody relying on the current behaviour can achieve > it by > > using BILINEAR. In addition GOOD is unchanged for any scales larger than > .75 > > or for a scale of exactly .5. > > > > Also despite their names, bilinear in no way would be considered "good" > or > > "best" by any sane person. We should not add illogical names (like > NEW_GOOD > > or whatever) just because of paranoia over back-compatibility. It is also > > highly desirable that the default actually be "good", this cannot be done > > unless GOOD is changed, or the default is changed to "NEW_GOOD". > > > > This change was made to Cairo over a year ago with no complaints (except > for > > speed issues, which this patch is necessary to solve by moving the fix > from > > Cairo to Pixman). Lets get out of the dark ages, and stop doing things > that > > are making open source desktops a laughingstock. > > > > Hi Bill, > I just now read the new emails (I was on PTO for the last week). > It seems you found some mistakes and you want to resend a new version. > Did I understand you correctly ? > If that is indeed the case, then I prefer to wait for that version > (v9), and skip reviewing v8. > Please ack this. > > Oded > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter
On Thu, Jan 7, 2016 at 11:44 PM, Pekka Paalanenwrote: > > Please keep in mind that the filters GOOD and BEST have been as is for > a long long time, AFAIU, so changing their behaviour now is likely not > a good idea. They are no longer "a good filter" and "whatever best > filter", but "the specific filter called GOOD" and "the specific filter > called BEST", in lack of documentation saying otherwise. > Both GOOD and BEST are identical to BILINEAR in the current version of Pixman. Therefore anybody relying on the current behaviour can achieve it by using BILINEAR. In addition GOOD is unchanged for any scales larger than .75 or for a scale of exactly .5. Also despite their names, bilinear in no way would be considered "good" or "best" by any sane person. We should not add illogical names (like NEW_GOOD or whatever) just because of paranoia over back-compatibility. It is also highly desirable that the default actually be "good", this cannot be done unless GOOD is changed, or the default is changed to "NEW_GOOD". This change was made to Cairo over a year ago with no complaints (except for speed issues, which this patch is necessary to solve by moving the fix from Cairo to Pixman). Lets get out of the dark ages, and stop doing things that are making open source desktops a laughingstock. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 03/13] pixman-image: Added GNUPLOT_OUTPUT compile-time option to view filters in gnuplot
On Mon, Jan 4, 2016 at 3:25 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Mon, Jan 4, 2016 at 5:12 AM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > If GNUPLOT_OUTPUT is set, then you can pipe the output of a pixman-using > program > > to gnuplot and get a continuously-updated plot of the horizontal filter. > This > > works well with demos/scale to test the filter generation. > > > > The plot is all the different subposition filters shuffled together. > This is > > misleading in a few cases: > > > > IMPULSE.BOX - goes up and down as the subfilters have different > numbers of non-zero samples > > IMPULSE.TRIANGLE - somewhat crooked for the same reason > > 1-wide filters - looks triangular, but a 1-wide box would be more > accurate > > --- > > pixman/pixman-image.c | 40 > > 1 file changed, 40 insertions(+) > > > > diff --git a/pixman/pixman-image.c b/pixman/pixman-image.c > > index 1ff1a49..69743c4 100644 > > --- a/pixman/pixman-image.c > > +++ b/pixman/pixman-image.c > > @@ -531,6 +531,46 @@ compute_image_info (pixman_image_t *image) > > > > image->common.flags = flags; > > image->common.extended_format_code = code; > > +/* If GNUPLOT_OUTPUT is set, then you can pipe the output of a > pixman-using program > > + * to gnuplot and get a continuously-updated plot of the horizontal > filter. This > > + * works well with demos/scale to test the filter generation. > > + * > > + * The plot is all the different subposition filters shuffled together. > This is > > + * misleading in a few cases: > > + * > > + * IMPULSE.BOX - goes up and down as the subfilters have different > numbers of non-zero samples > > + * IMPULSE.TRIANGLE - somewhat crooked for the same reason > > + * 1-wide filters - looks triangular, but a 1-wide box would be more > accurate > > + */ > > +/* #define GNUPLOT_OUTPUT 1 */ > > +#if GNUPLOT_OUTPUT > > +if ((flags & FAST_PATH_SEPARABLE_CONVOLUTION_FILTER) && > image->common.filter_params) { > > + const pixman_fixed_t* p = image->common.filter_params; > > + int width = pixman_fixed_to_int(p[0]); > > + int samples = 1 << pixman_fixed_to_int(p[2]); > > + int x,y; > > + p += 4; > > + printf("plot '-' with linespoints\n"); > > + printf("%g 0\n", - width * .5); > > + for (x = 0; x < width; ++x) { > > + for (y = 0; y < samples; ++y) { > > + int yy; > > + if (width & 1) > > + yy = y; > > + else if (y >= samples / 2) > > + yy = y - samples / 2; > > + else > > + yy = samples / 2 + y; > > + printf("%g %g\n", > > + x - width * .5 + (y + .5) * (1.0 / samples), > > + pixman_fixed_to_double(p[(yy + 1) * width - x - > 1])); > > + } > > + } > > + printf("%g 0\n", width * .5); > > + printf("e\n"); > > + fflush(stdout); > > +} > > +#endif > > } > > > > void > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > I get the big picture of this feature, and it seems useful, but we > need write this according to standards. > I would like to see the following changes: > > 1. We can do something more nice than a hard-coded #define. Let's add > a flag to configure.ac called "enable-gnuplot-output". For something > similar, see "enable-timers" in configure.ac. The default will be > "no". > That makes sense. I actually gave up trying to figure out how to set the CFLAGS and was forced to edit the source code to turn this on/off, which meant I often mistakenly checked the code in with it turned on. I guess configure is the way to go. > 2. Take your code and put it in a new function in pixman-utils.c, and > make sure the code inside the function is encompassed with > #ifdef/#endif around it. In case enable-gluplot-output wasn't called, > the function will be an empty function. > > 3. Call that function from compute_image_info. I guess you will want > to call it only if: > ((flags & FAST_PATH_SEPARABLE_CONVOLUTION_FILTER) && > image->common
Re: [Pixman] [PATCH 02/13] demos/scale: Added pulldown to choose PIXMAN_FILTER_* value
On Mon, Jan 4, 2016 at 1:24 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Mon, Jan 4, 2016 at 5:12 AM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > This allows testing of GOOD/BEST and to do comparisons between > > the basic filters and PIXMAN_FILTER_SEPARABLE_CONVOLUTION settings. > > You forgot to sign-off > Sorry, not sure what this means. Should I add something to the commit message? ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 13/15] pixman-filter: refactor cubic polynominal and don't range check
Indeed, further tests reveal there was a bug if one of the filters is IMPULSE. It was not sampling at the center of the filter, but instead offset by the width. I have a patch to fix this that will be in the next set. On 12/26/2015 08:05 PM, Bill Spitzak wrote: Sounds like I better look at this more carefully, it is quite possible it is producing bad filters but with small enough error that the images look OK. On Dec 23, 2015 5:25 AM, "Oded Gabbay" <oded.gab...@gmail.com <mailto:oded.gab...@gmail.com>> wrote: On Tue, Dec 22, 2015 at 9:01 PM, Bill Spitzak <spit...@gmail.com <mailto:spit...@gmail.com>> wrote: > > > On Tue, Dec 22, 2015 at 4:38 AM, Oded Gabbay <oded.gab...@gmail.com <mailto:oded.gab...@gmail.com>> wrote: >> >> On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com <mailto:spit...@gmail.com>> wrote: >> > From: Bill Spitzak <spit...@gmail.com <mailto:spit...@gmail.com>> >> > >> > The other filters do not check for x being in range, so there is >> > no reason for cubic to do so. >> >> This argument is a bit problematic. >> We could also argue that this filter was actually implemented >> correctly/more robust and we should add checks for x to the other >> filters. >> >> I fail to see how this saves us much except from removing a condition >> in a very specific path. >> >> Do you argue that ax will never ever be >=2 ? > > > Yes, because if that could happen, then out-of-range x could also be sent to > the other filter functions that are not doing the range check. > I run the scale demo, and added a printf everytime ax is >=2. I got a LOT of prints... So I don't think your argument is correct. > Adding range checks to all the other filters (especially the ones that > return constants) would add a bunch of conditions that are never used. Maybe, but it might be necessary to produce more accurate results ? Oded > >> >> >>Oded >> >> > --- >> > pixman/pixman-filter.c | 16 +++- >> > 1 file changed, 7 insertions(+), 9 deletions(-) >> > >> > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >> > index 7e10108..bf9dce3 100644 >> > --- a/pixman/pixman-filter.c >> > +++ b/pixman/pixman-filter.c >> > @@ -109,18 +109,16 @@ general_cubic (double x, double B, double C) >> > >> > if (ax < 1) >> > { >> > - return ((12 - 9 * B - 6 * C) * ax * ax * ax + >> > - (-18 + 12 * B + 6 * C) * ax * ax + (6 - 2 * B)) / 6; >> > -} >> > -else if (ax >= 1 && ax < 2) >> > -{ >> > - return ((-B - 6 * C) * ax * ax * ax + >> > - (6 * B + 30 * C) * ax * ax + (-12 * B - 48 * C) * >> > - ax + (8 * B + 24 * C)) / 6; >> > + return (((12 - 9 * B - 6 * C) * ax + >> > +(-18 + 12 * B + 6 * C)) * ax * ax + >> > + (6 - 2 * B)) / 6; >> > } >> > else >> > { >> > - return 0; >> > + return -B - 6 * C) * ax + >> > +(6 * B + 30 * C)) * ax + >> > + (-12 * B - 48 * C)) * ax + >> > + (8 * B + 24 * C)) / 6; >> > } >> > } >> > >> > -- >> > 1.9.1 >> > >> > ___ >> > Pixman mailing list >> > Pixman@lists.freedesktop.org <mailto:Pixman@lists.freedesktop.org> >> > http://lists.freedesktop.org/mailman/listinfo/pixman > > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 13/15] pixman-filter: refactor cubic polynominal and don't range check
Sounds like I better look at this more carefully, it is quite possible it is producing bad filters but with small enough error that the images look OK. On Dec 23, 2015 5:25 AM, "Oded Gabbay" <oded.gab...@gmail.com> wrote: > On Tue, Dec 22, 2015 at 9:01 PM, Bill Spitzak <spit...@gmail.com> wrote: > > > > > > On Tue, Dec 22, 2015 at 4:38 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> > >> On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > >> > From: Bill Spitzak <spit...@gmail.com> > >> > > >> > The other filters do not check for x being in range, so there is > >> > no reason for cubic to do so. > >> > >> This argument is a bit problematic. > >> We could also argue that this filter was actually implemented > >> correctly/more robust and we should add checks for x to the other > >> filters. > >> > >> I fail to see how this saves us much except from removing a condition > >> in a very specific path. > >> > >> Do you argue that ax will never ever be >=2 ? > > > > > > Yes, because if that could happen, then out-of-range x could also be > sent to > > the other filter functions that are not doing the range check. > > > I run the scale demo, and added a printf everytime ax is >=2. > I got a LOT of prints... > So I don't think your argument is correct. > > > Adding range checks to all the other filters (especially the ones that > > return constants) would add a bunch of conditions that are never used. > > Maybe, but it might be necessary to produce more accurate results ? > > Oded > > > > > >> > >> > >>Oded > >> > >> > --- > >> > pixman/pixman-filter.c | 16 +++- > >> > 1 file changed, 7 insertions(+), 9 deletions(-) > >> > > >> > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > >> > index 7e10108..bf9dce3 100644 > >> > --- a/pixman/pixman-filter.c > >> > +++ b/pixman/pixman-filter.c > >> > @@ -109,18 +109,16 @@ general_cubic (double x, double B, double C) > >> > > >> > if (ax < 1) > >> > { > >> > - return ((12 - 9 * B - 6 * C) * ax * ax * ax + > >> > - (-18 + 12 * B + 6 * C) * ax * ax + (6 - 2 * B)) / 6; > >> > -} > >> > -else if (ax >= 1 && ax < 2) > >> > -{ > >> > - return ((-B - 6 * C) * ax * ax * ax + > >> > - (6 * B + 30 * C) * ax * ax + (-12 * B - 48 * C) * > >> > - ax + (8 * B + 24 * C)) / 6; > >> > + return (((12 - 9 * B - 6 * C) * ax + > >> > +(-18 + 12 * B + 6 * C)) * ax * ax + > >> > + (6 - 2 * B)) / 6; > >> > } > >> > else > >> > { > >> > - return 0; > >> > + return -B - 6 * C) * ax + > >> > +(6 * B + 30 * C)) * ax + > >> > + (-12 * B - 48 * C)) * ax + > >> > + (8 * B + 24 * C)) / 6; > >> > } > >> > } > >> > > >> > -- > >> > 1.9.1 > >> > > >> > ___ > >> > Pixman mailing list > >> > Pixman@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/pixman > > > > > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 12/15] pixman-filter: Turn off subsampling when not necessary
On Tue, Dec 22, 2015 at 4:44 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > If sample is IMPULSE and reconstruct is BOX or IMPULSE the sub-pixel > > position of the sample is not relevant, so only one subsample is needed. > Why ? > I mean why it is not relevant ? and why only one subsample is needed ? > Because all the filters for all the subsample positions are the same (a single 1). Actually though, the code is indicating this is happening by returning a width of zero. But I think that is not necessary: if the filter width is 1, and the filters are normalized so they sum to 1, then all the filters must be equal and be a single 1. So I think the filter_width function can return 1 and the caller treats all values <= 1 as an indication that subsampling is not needed. Oded > > --- > > pixman/pixman-filter.c | 12 +++- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 64981cd..7e10108 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -230,6 +230,8 @@ filter_width (pixman_kernel_t reconstruct, > > pixman_kernel_t sample, > > double scale) > > { > > +if (reconstruct == PIXMAN_KERNEL_BOX && sample == > PIXMAN_KERNEL_IMPULSE) > > + return 0; > > return ceil (scale * filters[sample].width + > filters[reconstruct].width); > > } > > > > @@ -323,13 +325,13 @@ pixman_filter_create_separable_convolution (int > *n_values, > > int subsample_x, subsample_y; > > int width, height; > > > > -subsample_x = (1 << subsample_bits_x); > > -subsample_y = (1 << subsample_bits_y); > > - > > width = filter_width (reconstruct_x, sample_x, sx); > > -if (width < 1) width = 1; > > +if (width < 1) { width = 1; subsample_bits_x = 0; } > > height = filter_width (reconstruct_y, sample_y, sy); > > -if (height < 1) height = 1; > > +if (height < 1) { height = 1; subsample_bits_y = 0; } > > + > > +subsample_x = (1 << subsample_bits_x); > > +subsample_y = (1 << subsample_bits_y); > > > > *n_values = 4 + width * subsample_x + height * subsample_y; > > > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 11/15] pixman-filter: made IMPULSE.IMPULSE not produce a zero-wide filter
On Tue, Dec 22, 2015 at 4:21 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > With the other patch to put error on the center pixel, this produces > > the same result as BOX.IMPULSE filter. > > --- > > pixman/pixman-filter.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 00126cd..64981cd 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -327,7 +327,9 @@ pixman_filter_create_separable_convolution (int >*n_values, > > subsample_y = (1 << subsample_bits_y); > > > > width = filter_width (reconstruct_x, sample_x, sx); > > +if (width < 1) width = 1; > > Please put the assignment in a new line > Okay I will get these. > > > height = filter_width (reconstruct_y, sample_y, sy); > > +if (height < 1) height = 1; > > Same comment > > > > > *n_values = 4 + width * subsample_x + height * subsample_y; > > > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > I have the same request as with the other patch (center pixel). I can > see the visual difference - the picture in scale demo doesn't > disappear when reconstruct & sample are IMPLUSE - but I would like > some additional explanation to better understand. > > In which cases width/height are smaller than 1 ? How does that happen > ? How this patch solves it ? > The impulse filters have a width of zero (if you could actually plot them they are an infinitely thin and infinitely tall line with an area of 1.0). Once convolved with any other filter you will get a filter of width 1, with a value equal to the other filter at the center of the impulse filter. The convolving code detects the impulse filters and calculates this directly (it is possible the impulse filter calculating function, which returns 1.0, is not necessary as it is never called?). The proper result of impulse+impulse is 1.0 when the subsample is in the pixel center and 0.0 otherwise. But I don't think the result is useful (it means there are dots where the pixel centers line up). Also it would require the code to handle un-normalized filters which would complicate it a good deal. So I just made it produce the same as BOX.IMPULSE (ie nearest pixel). > >Oded > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter
I think the improvement is obvious if you check how much code is run to do the Simpson's integration. BOX.BOX will literally be the filter used almost always once this is finally fixed. On Tue, Dec 22, 2015 at 12:08 PM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Tue, Dec 22, 2015 at 9:44 PM, Bill Spitzak <spit...@gmail.com> wrote: > > Any test using Cairo is not using this code for scaling down (since it > uses > > it's own filter generator, or older Cairo which only used bilinear) so I > am > > not sure if this case is being hit. If GOOD in the future produces > BOX.BOX > > then this will be hit a lot more often. > > > OK, got it. Thanks. > > So do you have some other case where you can demonstrate the > effectiveness of this optimization ? > > If not, then I think we need to defer this patch until such a case > arises, e.g. what you wrote about GOOD producing BOX.BOX in the > future. > >Oded > > > > > > > On Tue, Dec 22, 2015 at 1:33 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> > >> On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > >> > From: Bill Spitzak <spit...@gmail.com> > >> > > >> > This is easy as the caller already intersected the two boxes, so > >> > the width is the integral. > >> > --- > >> > pixman/pixman-filter.c | 5 + > >> > 1 file changed, 5 insertions(+) > >> > > >> > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > >> > index 4aafa51..782f73d 100644 > >> > --- a/pixman/pixman-filter.c > >> > +++ b/pixman/pixman-filter.c > >> > @@ -182,6 +182,11 @@ integral (pixman_kernel_t reconstruct, double x1, > >> > assert (width == 0.0); > >> > return filters[sample].func (x2 / scale); > >> > } > >> > +else if (reconstruct == PIXMAN_KERNEL_BOX && sample == > >> > PIXMAN_KERNEL_BOX) > >> > +{ > >> > + assert (width <= 1.0); > >> > + return width; > >> > +} > >> > else if (sample == PIXMAN_KERNEL_IMPULSE) > >> > { > >> > assert (width == 0.0); > >> > -- > >> > 1.9.1 > >> > > >> > ___ > >> > Pixman mailing list > >> > Pixman@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/pixman > >> > >> > >> As Soren said in his original email, this specialized case is > >> justified if we can demonstrate an improvement in a real-world > >> use-case, while making sure there aren't any other regressions. > >> > >> Generally speaking, when adding specific conditions to optimize code > >> we need to see evidence that indeed the new code is faster in *most* > >> cases. This is because even if the added conditions improve > >> performance of a specific use-case, it might actually degrade > >> performance on most other cases as they now need to do additional > >> comparisons in every pass of this code. > >> > >> Therefore, I think we need to see some real numbers to accept this > patch. > >> > >> fyi, I did a cairo benchmark run (on the trimmed benchmarks), and it > >> was practically unchanged. When I checked the results with > >> "--min-change 1%", I got: > >> > >> Speedups > >> > >> image t-firefox-canvas-swscroll 691.34 (701.92 1.30%) -> 678.93 > >> (693.67 1.25%): 1.02x speedup > >> > >> image t-firefox-fishtank 1611.65 (1640.22 1.23%) -> 1591.66 > >> (1653.68 1.91%): 1.01x speedup > >> > >> Slowdowns > >> = > >> image t-gnome-system-monitor 886.06 (893.33 1.20%) -> 895.76 > >> (903.89 1.32%): 1.01x slowdown > >> > >> image t-firefox-fishbowl 3242.06 (3280.91 0.55%) -> 3284.20 > >> (3285.17 0.08%): 1.01x slowdown > >> > >> imaget-evolution 335.63 (337.11 0.28%) -> 340.48 > >> (352.97 1.62%): 1.01x slowdown > >> > >> imaget-xfce4-terminal-a1 554.95 (572.35 1.59%) -> 563.67 > >> (582.91 1.62%): 1.02x slowdown > >> > >> image t-gnome-terminal-vim 354.85 (358.67 0.67%) -> 364.49 > >> (366.12 0.33%): 1.03x slowdown > >> > >> Oded > > > > > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 10/15] pixman-filter: don't range-check impulse filter
The problem is that *Cairo* does not call this function. This is because Cairo already has my patches which work around the broken filtering by generating it's own filtering. The whole point of this series of patches is so that this work-around can be removed from Cairo. On Tue, Dec 22, 2015 at 12:12 PM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Tue, Dec 22, 2015 at 9:25 PM, Bill Spitzak <spit...@gmail.com> wrote: > > > > > > On Tue, Dec 22, 2015 at 2:32 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> > >> On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > >> > From: Bill Spitzak <spit...@gmail.com> > >> > > >> > The other filters don't range-check, so there is no need for this > >> > one to either. It is only called with x==0. > >> > >> Actually, I tried to stop at this function in gdb and didn't manage to > >> do it (using the scale demo). I then looked at the code and it seems > >> to me that the only way to reach this function is when both > >> reconstruction and sample kernels are IMPLUSE. That's because: > >> > >> 1. If both reconstruction and sample are *not* IMPLUSE, then of course > >> we won't reach it. > >> 2. If only one of them is IMPLUSE, than the code will immediately > >> return the value of the function of the other kernel, which is *not* > >> IMPLUSE. > >> > >> However, when I put both of them to IMPLUSE in the scale demo, the > >> picture simply disappears *and* the impluse_kernel is still not > >> reached. Actually, in that case, the integral() func is never reached > >> as well. > >> > >> What am I missing ? > > > > > > I believe at this point the calling code calculated a width of zero for > the > > filter, and this caused all kinds of problems. > > > > I think you are correct that in most or all versions of this code, that > > impulse function is never called, and it could be a null pointer instead. > > Well, I wouldn't go that far, but what I'm implying is maybe we can > defer this patch until a time when pixman's code will actually call > this function. Then, we can re-evaluate the patch based on the inputs > we will see. > >Oded > > >> > >> > >>Oded > >> > >> > --- > >> > pixman/pixman-filter.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > >> > index fbc657d..00126cd 100644 > >> > --- a/pixman/pixman-filter.c > >> > +++ b/pixman/pixman-filter.c > >> > @@ -45,7 +45,7 @@ typedef struct > >> > static double > >> > impulse_kernel (double x) > >> > { > >> > -return (x == 0.0)? 1.0 : 0.0; > >> > +return 1; > >> > } > >> > > >> > static double > >> > -- > >> > 1.9.1 > >> > > >> > ___ > >> > Pixman mailing list > >> > Pixman@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/pixman > > > > > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 10/15] pixman-filter: don't range-check impulse filter
On Tue, Dec 22, 2015 at 2:32 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > The other filters don't range-check, so there is no need for this > > one to either. It is only called with x==0. > > Actually, I tried to stop at this function in gdb and didn't manage to > do it (using the scale demo). I then looked at the code and it seems > to me that the only way to reach this function is when both > reconstruction and sample kernels are IMPLUSE. That's because: > > 1. If both reconstruction and sample are *not* IMPLUSE, then of course > we won't reach it. > 2. If only one of them is IMPLUSE, than the code will immediately > return the value of the function of the other kernel, which is *not* > IMPLUSE. > > However, when I put both of them to IMPLUSE in the scale demo, the > picture simply disappears *and* the impluse_kernel is still not > reached. Actually, in that case, the integral() func is never reached > as well. > > What am I missing ? > I believe at this point the calling code calculated a width of zero for the filter, and this caused all kinds of problems. I think you are correct that in most or all versions of this code, that impulse function is never called, and it could be a null pointer instead. > >Oded > > > --- > > pixman/pixman-filter.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index fbc657d..00126cd 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -45,7 +45,7 @@ typedef struct > > static double > > impulse_kernel (double x) > > { > > -return (x == 0.0)? 1.0 : 0.0; > > +return 1; > > } > > > > static double > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 13/15] pixman-filter: refactor cubic polynominal and don't range check
On Tue, Dec 22, 2015 at 4:38 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > The other filters do not check for x being in range, so there is > > no reason for cubic to do so. > > This argument is a bit problematic. > We could also argue that this filter was actually implemented > correctly/more robust and we should add checks for x to the other > filters. > > I fail to see how this saves us much except from removing a condition > in a very specific path. > > Do you argue that ax will never ever be >=2 ? > Yes, because if that could happen, then out-of-range x could also be sent to the other filter functions that are not doing the range check. Adding range checks to all the other filters (especially the ones that return constants) would add a bunch of conditions that are never used. > >Oded > > > --- > > pixman/pixman-filter.c | 16 +++- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 7e10108..bf9dce3 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -109,18 +109,16 @@ general_cubic (double x, double B, double C) > > > > if (ax < 1) > > { > > - return ((12 - 9 * B - 6 * C) * ax * ax * ax + > > - (-18 + 12 * B + 6 * C) * ax * ax + (6 - 2 * B)) / 6; > > -} > > -else if (ax >= 1 && ax < 2) > > -{ > > - return ((-B - 6 * C) * ax * ax * ax + > > - (6 * B + 30 * C) * ax * ax + (-12 * B - 48 * C) * > > - ax + (8 * B + 24 * C)) / 6; > > + return (((12 - 9 * B - 6 * C) * ax + > > +(-18 + 12 * B + 6 * C)) * ax * ax + > > + (6 - 2 * B)) / 6; > > } > > else > > { > > - return 0; > > + return -B - 6 * C) * ax + > > +(6 * B + 30 * C)) * ax + > > + (-12 * B - 48 * C)) * ax + > > + (8 * B + 24 * C)) / 6; > > } > > } > > > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 09/15] pixman-filter: put filter error on center pixel
This is forcing the filter to be normalized (sum to 1.0) despite any math errors. p is a post-increment pointer used to store the filter values, so it now points after the last filter value. The filter is summed and the difference from 1.0 is then added to the middle pixel with this code. If the width is an odd number such as 5, the filter is stored in f[0]..f[4], and p points at f[5]. The old code would add the extra error value to f[3], which is not the center sample. The new code adds it to f[2]. (If the width is even such as 4, then both the old and new code add the error to the f[2] which is the pixel to the right of the center.) The mistake was only visible in the width==1 case. Some combinations of filters produced a sample of 0, and then calculated the error as 1.0. The old code would put the error on f[1] (past the end). The new code puts it on f[0], thus producing a value of 1.0. On Tue, Dec 22, 2015 at 2:41 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > Any error in filter normalization is placed on the center of odd-sized > filters, > > rather than 1 pixel to the right. > > > > In particular this fixes the 1-wide filters produced by impulse sampling > > so they are 1.0 rather than 0.0. > > --- > > pixman/pixman-filter.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 0cd4a68..fbc657d 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -299,7 +299,7 @@ create_1d_filter (int width, > > } > > > > if (new_total != pixman_fixed_1) > > - *(p - width / 2) += (pixman_fixed_1 - new_total); > > + *(p - (width + 1) / 2) += (pixman_fixed_1 - new_total); > > } > > } > > > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > > I see that the result is indeed better (in the scale demo), but do you > mind explaining a bit more about the underlying of this patch ? > I indeed see that the width is 1, so without your patch, the new value > is written to *p and with your patch, it is written to *(p-1). But I > have no idea what that means :( > > Oded > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 08/15] pixman-filter: Don't recurse unnecessarily.
On Tue, Dec 22, 2015 at 1:38 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > Only LINEAR is not differentiable at zero, > > I'm sorry, I don't understand this sentence. Could you please give me > a more detailed explanation + reference ? > All the other filter functions have a continuous first derivative over their entire range. The LINEAR function is a triangle and the first derivative changes from +1 to -1 at 0.0. I believe Søren was concerned that the Simpson's integration would not work at this point. He solved this by splitting the integral into 2 or 3 at the zeros, so the integration is not done across these points. I figured I should keep this, though I suspect the error is really invisibly tiny. Apparently Simpsons integration will have some error for any function where the third derivative is not constant, which is true for all the filters here except box. But the error is really really tiny and was being ignored for all the other filters, and it may be ok to ignore it here too. Thanks, > > Oded > > > so only do the recursive split of the integral for it. > > --- > > pixman/pixman-filter.c | 34 +- > > 1 file changed, 17 insertions(+), 17 deletions(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 782f73d..0cd4a68 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -160,38 +160,38 @@ integral (pixman_kernel_t reconstruct, double x1, > > pixman_kernel_t sample, double scale, double x2, > > double width) > > { > > +if (reconstruct == PIXMAN_KERNEL_IMPULSE) > > +{ > > + assert (width == 0.0); > > + return filters[sample].func (x2 / scale); > > +} > > +else if (reconstruct == PIXMAN_KERNEL_BOX && sample == > PIXMAN_KERNEL_BOX) > > +{ > > + assert (width <= 1.0); > > + return width; > > +} > > +else if (sample == PIXMAN_KERNEL_IMPULSE) > > +{ > > + assert (width == 0.0); > > + return filters[reconstruct].func (x1); > > +} > > /* If the integration interval crosses zero, break it into > > * two separate integrals. This ensures that filters such > > * as LINEAR that are not differentiable at 0 will still > > * integrate properly. > > */ > > -if (x1 < 0 && x1 + width > 0) > > +else if (reconstruct == PIXMAN_KERNEL_LINEAR && x1 < 0 && x1 + > width > 0) > > { > > return > > integral (reconstruct, x1, sample, scale, x2, - x1) + > > integral (reconstruct, 0, sample, scale, x2 - x1, width + > x1); > > } > > -else if (x2 < 0 && x2 + width > 0) > > +else if (sample == PIXMAN_KERNEL_LINEAR && x2 < 0 && x2 + width > 0) > > { > > return > > integral (reconstruct, x1, sample, scale, x2, - x2) + > > integral (reconstruct, x1 - x2, sample, scale, 0, width + > x2); > > } > > -else if (reconstruct == PIXMAN_KERNEL_IMPULSE) > > -{ > > - assert (width == 0.0); > > - return filters[sample].func (x2 / scale); > > -} > > -else if (reconstruct == PIXMAN_KERNEL_BOX && sample == > PIXMAN_KERNEL_BOX) > > -{ > > - assert (width <= 1.0); > > - return width; > > -} > > -else if (sample == PIXMAN_KERNEL_IMPULSE) > > -{ > > - assert (width == 0.0); > > - return filters[reconstruct].func (x1); > > -} > > else > > { > > /* Integration via Simpson's rule */ > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 07/15] pixman-filter: Speed up the BOX+BOX filter
Any test using Cairo is not using this code for scaling down (since it uses it's own filter generator, or older Cairo which only used bilinear) so I am not sure if this case is being hit. If GOOD in the future produces BOX.BOX then this will be hit a lot more often. On Tue, Dec 22, 2015 at 1:33 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > This is easy as the caller already intersected the two boxes, so > > the width is the integral. > > --- > > pixman/pixman-filter.c | 5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 4aafa51..782f73d 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -182,6 +182,11 @@ integral (pixman_kernel_t reconstruct, double x1, > > assert (width == 0.0); > > return filters[sample].func (x2 / scale); > > } > > +else if (reconstruct == PIXMAN_KERNEL_BOX && sample == > PIXMAN_KERNEL_BOX) > > +{ > > + assert (width <= 1.0); > > + return width; > > +} > > else if (sample == PIXMAN_KERNEL_IMPULSE) > > { > > assert (width == 0.0); > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > > As Soren said in his original email, this specialized case is > justified if we can demonstrate an improvement in a real-world > use-case, while making sure there aren't any other regressions. > > Generally speaking, when adding specific conditions to optimize code > we need to see evidence that indeed the new code is faster in *most* > cases. This is because even if the added conditions improve > performance of a specific use-case, it might actually degrade > performance on most other cases as they now need to do additional > comparisons in every pass of this code. > > Therefore, I think we need to see some real numbers to accept this patch. > > fyi, I did a cairo benchmark run (on the trimmed benchmarks), and it > was practically unchanged. When I checked the results with > "--min-change 1%", I got: > > Speedups > > image t-firefox-canvas-swscroll 691.34 (701.92 1.30%) -> 678.93 > (693.67 1.25%): 1.02x speedup > > image t-firefox-fishtank 1611.65 (1640.22 1.23%) -> 1591.66 > (1653.68 1.91%): 1.01x speedup > > Slowdowns > = > image t-gnome-system-monitor 886.06 (893.33 1.20%) -> 895.76 > (903.89 1.32%): 1.01x slowdown > > image t-firefox-fishbowl 3242.06 (3280.91 0.55%) -> 3284.20 > (3285.17 0.08%): 1.01x slowdown > > imaget-evolution 335.63 (337.11 0.28%) -> 340.48 > (352.97 1.62%): 1.01x slowdown > > imaget-xfce4-terminal-a1 554.95 (572.35 1.59%) -> 563.67 > (582.91 1.62%): 1.02x slowdown > > image t-gnome-terminal-vim 354.85 (358.67 0.67%) -> 364.49 > (366.12 0.33%): 1.03x slowdown > > Oded > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Plan to release final development version before stable branch
I suspect I will need to check the X server version, actually. That seems to be what the other tests are doing. My main concern is to figure out *which* version this (will) happen in. I am hoping this can be known before code release so Cairo can be updated to use it at the same time. On Sun, Dec 20, 2015 at 5:58 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Wed, Dec 16, 2015 at 4:41 AM, Bill Spitzak <spit...@gmail.com> wrote: > > > > > > On Tue, Dec 15, 2015 at 1:29 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> > >> On Sat, Dec 12, 2015 at 9:10 PM, Bill Spitzak <spit...@gmail.com> > wrote: > >> > > >> > > >> > On Sat, Dec 12, 2015 at 10:37 AM, Oded Gabbay <oded.gab...@gmail.com> > >> > wrote: > >> >> > >> >> On Sat, Dec 12, 2015 at 8:34 PM, Bill Spitzak <spit...@gmail.com> > >> >> wrote: > >> >> > On Fri, Dec 11, 2015 at 4:15 AM, Oded Gabbay < > oded.gab...@gmail.com> > >> >> > wrote: > >> >> >> > >> >> >> On Thu, Dec 10, 2015 at 11:19 PM, Bill Spitzak <spit...@gmail.com > > > >> >> >> wrote: > >> >> >> > Can you include my patches to fix the filtering? They have been > >> >> >> > posted > >> >> >> > for a > >> >> >> > long time now. > >> >> >> > > >> >> >> > The last patch makes GOOD/BEST use filtering for scaling images > >> >> >> > down. > >> >> >> > This > >> >> >> > matches the current Cairo behavior and would allow Cairo to use > >> >> >> > the > >> >> >> > pixman > >> >> >> > backend rather than doing an image fallback for any image > scaling > >> >> >> > smaller > >> >> >> > than .75. It also contains a bunch of minor optimizaion and > filter > >> >> >> > selection > >> >> >> > tweaks that makes the output somewhat better than current Cairo. > >> >> >> > > >> >> >> Hi Bill, > >> >> >> > >> >> >> Unfortunately, I don't see anyone reviewed your patches, and from > >> >> >> what > >> >> >> I heard, those are quite significant changes. > >> >> >> > >> >> >> It's a shame you didn't bring this up when I did the first > >> >> >> development > >> >> >> release 4 months ago. Then we had enough time to check and test > it. > >> >> >> I'm quite hesitant of including such changes right before the > final > >> >> >> development version, even with a review. > >> >> > > >> >> > > >> >> > I did send email on May 22, 2015, in response to your comments. > >> >> > >> >> That's strange, because I only started working on pixman during June > of > >> >> 2015... > >> > > >> > > >> > You are right. That was just a general email I sent trying to get > >> > somebody > >> > to look at the patches. Searching in the history I found 3 of these. > >> > > >> >> > >> >> > >> >> > >> >> > > >> >> >> I suggest that you try to contact one of pixman's veterans (Soren, > >> >> >> Siarhei, Matt, Pekka, Ben) offline and ask them nicely to at least > >> >> >> skim over the patches and give a high-level opinion about the > >> >> >> series. > >> >> > > >> >> > > >> >> > These were discussed with Soren before. He disagreed with my > previous > >> >> > version because I changed to a single filter calculation rather > than > >> >> > his > >> >> > pair of filters being convoluted. This version preserves the pair > of > >> >> > filters, with some fixes of bugs that caused artifacts in the > >> >> > resulting > >> >> > filters. I'm sending email directly in case they are not reading > the > >> >> > pixman > >> >> > list. > >> >> > >> >> Could you send me those emails ? > >> > > >> > > >> > I forwarded the big one from
Re: [Pixman] [PATCH 01/15] demos/scale: Compute filter size using boundary of xformed ellipse, not rectangle
Well that was a pain to figure out again (it is tricky to get p and x and y cancelled out of the calculation) but does this make any sense: Calculate bounding box of radius-1 circle in xy transformed to uv: Transform x,y to u,v by this matrix calculation: |u| |a c| |x| |v| = |b d|*|y| Horizontal component: u = ax+cy (1) x,y describes a radius-1 circle, p is angle to the point: p = 0..2*pi x = cos(p) y = sin(p) x^2+y^2 = 1 dx/dp = -sin(p) = -y dy/dp = cos(p) = x Figure out derivative of (1) relative to p: du/dp = a(dx/dp) + c(dy/dp) = -ay + cx The min and max u are when du/dp is zero: -ay + cx = 0 cx = ay c = ay/x (2) y = cx/a (3) Substitute (2) into (1) and simplify: u = ax + ay^2/x = a(x^2+y^2)/x = a/x (because x^2+y^2 = 1) x = a/u (4) Substitute (4) into (3) and simplify: y = c(a/u)/a y = c/u (5) Square (4) and (5) and add: x^2+y^2 = (a^2+c^2)/u^2 But x^2+y^2 is 1: 1 = (a^2+c^2)/u^2 u^2 = a^2+c^2 u = hypot(a,c) Similarily the max/min of v is at: v = hypot(b,d) On Thu, Dec 17, 2015 at 1:52 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > This is much more accurate and less blurry. In particular the filtering > does > > not change as the image is rotated. > > --- > > demos/scale.c | 43 ++- > > 1 file changed, 2 insertions(+), 41 deletions(-) > > > > diff --git a/demos/scale.c b/demos/scale.c > > index d00307e..71c7791 100644 > > --- a/demos/scale.c > > +++ b/demos/scale.c > > @@ -55,50 +55,11 @@ get_widget (app_t *app, const char *name) > > return widget; > > } > > > > -static double > > -min4 (double a, double b, double c, double d) > > -{ > > -double m1, m2; > > - > > -m1 = MIN (a, b); > > -m2 = MIN (c, d); > > -return MIN (m1, m2); > > -} > > - > > -static double > > -max4 (double a, double b, double c, double d) > > -{ > > -double m1, m2; > > - > > -m1 = MAX (a, b); > > -m2 = MAX (c, d); > > -return MAX (m1, m2); > > -} > > - > > static void > > compute_extents (pixman_f_transform_t *trans, double *sx, double *sy) > > { > > -double min_x, max_x, min_y, max_y; > > -pixman_f_vector_t v[4] = > > -{ > > - { { 1, 1, 1 } }, > > - { { -1, 1, 1 } }, > > - { { -1, -1, 1 } }, > > - { { 1, -1, 1 } }, > > -}; > > - > > -pixman_f_transform_point (trans, [0]); > > -pixman_f_transform_point (trans, [1]); > > -pixman_f_transform_point (trans, [2]); > > -pixman_f_transform_point (trans, [3]); > > - > > -min_x = min4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); > > -max_x = max4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); > > -min_y = min4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); > > -max_y = max4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); > > - > > -*sx = (max_x - min_x) / 2.0; > > -*sy = (max_y - min_y) / 2.0; > > +*sx = hypot (trans->m[0][0], trans->m[0][1]) / trans->m[2][2]; > > +*sy = hypot (trans->m[1][0], trans->m[1][1]) / trans->m[2][2]; > > } > > > > typedef struct > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > Could you please add some comment in the code about where this > calculation comes from (reference to some mathematical > equation/proof), and detail the mapping of the variables in the code > to the arguments of the mathematical equation ? > > Otherwise, this patch is: > > Reviewed-by: Oded Gabbay <oded.gab...@gmail.com> > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 06/15] pixman-filter: reduced number of samples in Simpson's integration
Ok to squash them together. Do you want me to do that? It actually does not increase the runtime, because the two loops are only adding every *other* sample. Thus the same number of samples are computed. On Thu, Dec 17, 2015 at 1:23 PM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: > > From: Bill Spitzak <spit...@gmail.com> > > > > With the cubic fix this is plenty accurate enough, far in excess of the > pixman > > fixed-point error limit. Likely even 16 samples is too many. > > --- > > pixman/pixman-filter.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c > > index 7c1da0d..4aafa51 100644 > > --- a/pixman/pixman-filter.c > > +++ b/pixman/pixman-filter.c > > @@ -190,7 +190,7 @@ integral (pixman_kernel_t reconstruct, double x1, > > else > > { > > /* Integration via Simpson's rule */ > > -#define N_SEGMENTS 128 > > +#define N_SEGMENTS 16 > > #define SAMPLE(a1, a2) \ > > (filters[reconstruct].func ((a1)) * filters[sample].func ((a2) / > scale)) > > > > -- > > 1.9.1 > > > > ___ > > Pixman mailing list > > Pixman@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/pixman > > I think it is better to just squash this patch into the previous one, > as it closely related and actually makes more sense to put them > together so we can see the run time hasn't increased but actually > decreased. > > Oded > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 05/15] pixman-filter: Correct Simpsons integration
Best one I think: http://www.intmath.com/integration/6-simpsons-rule.php On Thu, Dec 17, 2015 at 1:50 PM, Bill Spitzak <spit...@gmail.com> wrote: > This was based on looking up Simpson's integration on the web, from the > wikipedia page and another page I found. > > It cuts the samples into sets of 3, with an overlap of 1. Each set then > weighs 1,4,1 in the average, to simulate the weight of the control points > of a cubic curve. Since the overlapping samples of 1 add to 2 this results > in 1,4,2,4,2,...4,1 as the weights. As there are two points per set and > the total weight is 1+4+1=6, you divide the full sum by 6/2 = 3. > > It appears this implementation attempted to overlap them by 2, resulting > in weights of 1,5,6,...6,5,1. However this is very close to a flat average > of all the points. Also this is a total of 6 for every point so the divisor > should be 6, but it was left at 3. > > Based on my reading the new version is correct. However I have not been > able to see any visible difference in the filtering even if I reduce the > number of samples to 3. > > > On Thu, Dec 17, 2015 at 10:21 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> On Thu, Dec 17, 2015 at 8:20 PM, Oded Gabbay <oded.gab...@gmail.com> >> wrote: >> > On Sat, Dec 12, 2015 at 8:06 PM, <spit...@gmail.com> wrote: >> >> From: Bill Spitzak <spit...@gmail.com> >> >> >> >> Simpsons uses cubic curve fitting, with 3 samples defining each cubic. >> This >> >> makes the weights of the samples be in a pattern of 1,4,2,4,2...4,1, >> and then >> >> dividing the result by 3. >> >> >> >> The previous code was using weights of 1,2,6,6...6,2,1 which produced >> about 2x >> >> the correct value, as it was still dividing by 3. The filter >> normalization >> >> removed this error. Also this is effectively a linear interpolation >> except for >> >> the ends. >> >> --- >> >> pixman/pixman-filter.c | 11 +++ >> >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c >> >> index 15f9069..7c1da0d 100644 >> >> --- a/pixman/pixman-filter.c >> >> +++ b/pixman/pixman-filter.c >> >> @@ -204,11 +204,14 @@ integral (pixman_kernel_t reconstruct, double x1, >> >> { >> >> double a1 = x1 + h * i; >> >> double a2 = x2 + h * i; >> >> + s += 4 * SAMPLE(a1, a2); >> >> + } >> >> >> >> - s += 2 * SAMPLE (a1, a2); >> >> - >> >> - if (i >= 2 && i < N_SEGMENTS - 1) >> >> - s += 4 * SAMPLE (a1, a2); >> >> + for (i = 2; i < N_SEGMENTS; i += 2) >> >> + { >> >> + double a1 = x1 + h * i; >> >> + double a2 = x2 + h * i; >> >> + s += 2 * SAMPLE(a1, a2); >> >> } >> >> >> >> s += SAMPLE (x1 + width, x2 + width); >> >> -- >> >> 1.9.1 >> >> >> >> ___ >> >> Pixman mailing list >> >> Pixman@lists.freedesktop.org >> >> http://lists.freedesktop.org/mailman/listinfo/pixman >> > >> > You say: >> > >> > "The filter normalization removed this error. Also this is effectively >> > a linear interpolation except for the ends." >> > >> > So if the error was removed, why is this change needed ? I can see it >> > is more accurate (similar to the Simpson equation), but it also causes >> > the code to run over the loop twice. >> > >> > Do you have some example we can see the difference ? >> > >> > >> > Oded >> >> OK, now I see that in the next patch, you reduce the samples from 128 >> to 16, so we are now running less iterations. >> I still would be happy to see an example with my own eyes where this >> makes a difference. >> >> Oded >> > > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Plan to release final development version before stable branch
On Tue, Dec 15, 2015 at 1:29 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Sat, Dec 12, 2015 at 9:10 PM, Bill Spitzak <spit...@gmail.com> wrote: > > > > > > On Sat, Dec 12, 2015 at 10:37 AM, Oded Gabbay <oded.gab...@gmail.com> > wrote: > >> > >> On Sat, Dec 12, 2015 at 8:34 PM, Bill Spitzak <spit...@gmail.com> > wrote: > >> > On Fri, Dec 11, 2015 at 4:15 AM, Oded Gabbay <oded.gab...@gmail.com> > >> > wrote: > >> >> > >> >> On Thu, Dec 10, 2015 at 11:19 PM, Bill Spitzak <spit...@gmail.com> > >> >> wrote: > >> >> > Can you include my patches to fix the filtering? They have been > >> >> > posted > >> >> > for a > >> >> > long time now. > >> >> > > >> >> > The last patch makes GOOD/BEST use filtering for scaling images > down. > >> >> > This > >> >> > matches the current Cairo behavior and would allow Cairo to use the > >> >> > pixman > >> >> > backend rather than doing an image fallback for any image scaling > >> >> > smaller > >> >> > than .75. It also contains a bunch of minor optimizaion and filter > >> >> > selection > >> >> > tweaks that makes the output somewhat better than current Cairo. > >> >> > > >> >> Hi Bill, > >> >> > >> >> Unfortunately, I don't see anyone reviewed your patches, and from > what > >> >> I heard, those are quite significant changes. > >> >> > >> >> It's a shame you didn't bring this up when I did the first > development > >> >> release 4 months ago. Then we had enough time to check and test it. > >> >> I'm quite hesitant of including such changes right before the final > >> >> development version, even with a review. > >> > > >> > > >> > I did send email on May 22, 2015, in response to your comments. > >> > >> That's strange, because I only started working on pixman during June of > >> 2015... > > > > > > You are right. That was just a general email I sent trying to get > somebody > > to look at the patches. Searching in the history I found 3 of these. > > > >> > >> > >> > >> > > >> >> I suggest that you try to contact one of pixman's veterans (Soren, > >> >> Siarhei, Matt, Pekka, Ben) offline and ask them nicely to at least > >> >> skim over the patches and give a high-level opinion about the series. > >> > > >> > > >> > These were discussed with Soren before. He disagreed with my previous > >> > version because I changed to a single filter calculation rather than > his > >> > pair of filters being convoluted. This version preserves the pair of > >> > filters, with some fixes of bugs that caused artifacts in the > resulting > >> > filters. I'm sending email directly in case they are not reading the > >> > pixman > >> > list. > >> > >> Could you send me those emails ? > > > > > > I forwarded the big one from him and my response. The patches I have had > > since then I believe address his concerns and preserve the 2-filter > > convolution api, they are just bug fixes and some efficiency changes. > >> > >> > >> >> > >> >> > >> >> Also, check if you need to rebase the patches against current pixman > >> >> and if so, maybe send the series again. It might stir up a > discussion. > >> > > >> > > >> > The patches applied to the newest version without any conflicts and my > >> > test > >> > programs still work. I have resent them to the pixman mailing list. > >> >> > >> > >> Great! > >> > >> >> > >> >> I'm willing to review them in terms of correctness and code style, > but > >> >> I'm not veteran enough in pixman to give an opinion on the underlying > >> >> changes (which is the most important issue). > >> > > >> > > >> > Anything would be great. > >> > > >> > I believe these work well and have been using them for a while. This > >> > would > >> > allow the removal of redundant code in Cairo, and would allow 2-pass > >> > filtering to be done at some
Re: [Pixman] Plan to release final development version before stable branch
On Fri, Dec 11, 2015 at 4:15 AM, Oded Gabbay <oded.gab...@gmail.com> wrote: > On Thu, Dec 10, 2015 at 11:19 PM, Bill Spitzak <spit...@gmail.com> wrote: > > Can you include my patches to fix the filtering? They have been posted > for a > > long time now. > > > > The last patch makes GOOD/BEST use filtering for scaling images down. > This > > matches the current Cairo behavior and would allow Cairo to use the > pixman > > backend rather than doing an image fallback for any image scaling smaller > > than .75. It also contains a bunch of minor optimizaion and filter > selection > > tweaks that makes the output somewhat better than current Cairo. > > > Hi Bill, > > Unfortunately, I don't see anyone reviewed your patches, and from what > I heard, those are quite significant changes. > > It's a shame you didn't bring this up when I did the first development > release 4 months ago. Then we had enough time to check and test it. > I'm quite hesitant of including such changes right before the final > development version, even with a review. > I did send email on May 22, 2015, in response to your comments. I suggest that you try to contact one of pixman's veterans (Soren, > Siarhei, Matt, Pekka, Ben) offline and ask them nicely to at least > skim over the patches and give a high-level opinion about the series. > These were discussed with Soren before. He disagreed with my previous version because I changed to a single filter calculation rather than his pair of filters being convoluted. This version preserves the pair of filters, with some fixes of bugs that caused artifacts in the resulting filters. I'm sending email directly in case they are not reading the pixman list. > > Also, check if you need to rebase the patches against current pixman > and if so, maybe send the series again. It might stir up a discussion. > The patches applied to the newest version without any conflicts and my test programs still work. I have resent them to the pixman mailing list. > > I'm willing to review them in terms of correctness and code style, but > I'm not veteran enough in pixman to give an opinion on the underlying > changes (which is the most important issue). > Anything would be great. I believe these work well and have been using them for a while. This would allow the removal of redundant code in Cairo, and would allow 2-pass filtering to be done at some point in the future, which would really improve pixman performance. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Plan to release final development version before stable branch
Can you include my patches to fix the filtering? They have been posted for a long time now. The last patch makes GOOD/BEST use filtering for scaling images down. This matches the current Cairo behavior and would allow Cairo to use the pixman backend rather than doing an image fallback for any image scaling smaller than .75. It also contains a bunch of minor optimizaion and filter selection tweaks that makes the output somewhat better than current Cairo. On Wed, Dec 9, 2015 at 12:28 AM, Oded Gabbaywrote: > Hi All, > > I'm planning to release a new pixman development version (0.33.6) on > 12/16. Immediately after that, I'm going to create a new stable branch > - 0.34. > > Once that happen, only fixes will be accepted to that branch until the > stable release sometime in late January. Regular development will > continue in master. > > If you have some pending patches please shout. > > Thanks, > > Oded > ___ > Pixman mailing list > Pixman@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/pixman > ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’
On Mon, Sep 21, 2015 at 10:07 PM, Søren Sandmannwrote: > Sure. The extra width check can't harm. > Actually it can, because it implies that such values *can* arrive at this function, leading programmers to add tests to the calling functions, thus leading to a large number of tests for conditions that cannot happen. I would prefer to see an assert(width > 0) there. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Benchmarked: [PATCH 1/4] Change conditions for setting FAST_PATH_SAMPLES_COVER_CLIP flags
On Wed, Sep 16, 2015 at 7:30 AM, Ben Avisonwrote: Just by thinking things through, I realised that we would regularly fail > to hit COVER paths if Pixman's caller set the scale factors such that the > centre of the outermost destination pixels aligned with the centre of the > outermost source pixels. There has been some argument about whether this > is representative of how Pixman should be used. I happen to think this is > a perfectly reasonable thing to expect Pixman to support, but there are > other models you can follow, notably setting the scale factors such that > the outer edges of the outermost destination pixels align to the outer > edges of the outermost source pixels. If the Cairo traces are using this > latter model, then it's understandable if you aren't hitting the edge > case that I'm concerned about very often. > There are currently very good reasons for clients to use the first model: it is a method of removing undesirable "fuzzy edges" from zoomed-in images. The alternative of scaling the outer edge, while not hitting this fast path, will not hit the other fast path either! (the other fast path is the one that allows a zero-weighted pixel at one end to be read from the image). Therefore I believe there is actual usage of this new fast path, while there is almost zero usage of the old fast path. This is the main reason I think the current flag should be removed, and a new one indicating that all the non-zero samples are inside the image, added. This second one will actually be used by real code. NOTE: the method of scaling the centers of the pixels is generally wrong. The math is wonky: does scaling an image by 2 produce a 2*w-1 image, or does it spread the pixels slightly more than 2 apart? Programs are going to disagree. And you have to alter the math as soon as the scale goes below 1 or you will get fuzzy edges again. And lots of code are going to get coordinates in the resulting image wrong, resulting in annoying problems like overlaid lines not lining up or shifting their alignment from one side to another. I very much believe Cairo should change how CAIRO_EXTEND_NONE is done. It should instead mean that the path is intersected with a quadrilateral that is the transform of the outer edge of the source, then drawing as though using CAIRO_EXTEND_REPEAT. This is what most users of scaling expect, and anybody wanting the previous effect can get it by adding a black pixel to the outer edge of their source surface. Now it is true that this result can be achieved with Cairo right now. The reason for making this the default is because non-experts are unlikely to figure this out. Currently filling a path equal to the source boundary results in a double-mulitplication of the edge pixels, a subtle error that can be very frustrating when graphics gets more complex. Making this error not happen by default would be very helpful. I also believe making this the default will result in faster implementations, as EXTEND_REPEAT fast paths can be used, and this is often directly supported by graphics hardware. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 1/4] Change conditions for setting FAST_PATH_SAMPLES_COVER_CLIP flags
On Mon, Sep 14, 2015 at 11:52 AM, Søren Sandmannwrote: > > A separate possibility is a flag that says "all pixels whose weights are > non-zero are inside the borders of the source image". Is this useful > information? It might be, and if so, it could be conveyed through some > new flag, though I'd echo Siarhei's comment about whether this is > something that happens in practice. > I believe this *is* what happens in practice, much more often. The clip regions are not random, they are chosen by programmers for specific purposes. One thing that is wanted is to scale images up and preserve sharp edges. In Cairo this requires trimming the partial pixels off the edge. This will produce a clip that will turn on this flag. The alternative version of the flag will require the program to clip off at least one opaque pixel from two edges for scale factors less than 2. There is far less reason for a program to do that. Therefore I think this version of the flag will actually be used far more often, easily making up for the expense of adding the test for the zero-weight pixel to the bilinear fast paths. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 1/4] Change conditions for setting FAST_PATH_SAMPLES_COVER_CLIP flags
On Thu, Sep 10, 2015 at 9:35 AM, Ben Avisonwrote: > On Thu, 10 Sep 2015 12:46:50 +0100, Pekka Paalanen > wrote: > >> you're right, documenting this is important. However, I think this >> particular patch is not the best place, and here is why. >> >> When we recently discussed this, both I and Siarhei had the opinion >> that this needs to be done in two separate steps: >> >> 1. Remove the useless 8e fuzz margins. >> >> 2. Change the meaning of the COVER_CLIP_BILINEAR flag so that it is no >>longer safe for fetchers to always fetch a 2x2 pixel block. >> > This sounds exactly right to me. Another way to say it is that it is safe to fetch all pixels that have a non-zero weight. Certain bilinear positions will produce 2x2 blocks that contain up to 3 pixels with a zero weight, those pixels may be outside the source clip even when this flag is on. > I sense we're taking a slightly different perspective on the problem > here. I don't really see these two steps as different in spirit. In the > first one, the flag calculation was allowing extra space to permit the > corresponding fast path to be a bit sloppy with its coordinate > transformations. In the second one, the flag calculation was allowing > extra space to permit the corresponding fast path to be a bit sloppy with > loading data that it doesn't need. Apart from meaning that less efficient > fast paths sometimes get used, this also means a lot of unnecessary cache > lines get loaded in many cases, which has got to hurt performance. > I believe you are confusing the implementation of the bilinear with this flag. For a coordinate of .5, pixel 0 will have a weight of 1.0, and all other pixels will have a weight of 0.0. This includes both the pixel at -1 and the pixel at 1. They both have a weight of zero. It is useful for a bilinear algorithm to think about pairs of pixels, and depending on the implementation the pair produced for the .5 coordinate may be pixels 0 and 1, or pixels -1 and 0. However this does not change the setting of COVER_CLIP_BILINEAR! No pixel has changed it's weight, both pixel -1 and 1 remain with a weight of zero, no matter which is chosen for the pair. I think you are right that there are reasons for the bilinear *implementation* to round down, so the weighted-zero pixel is at the lower coordinate. This is because the special code to avoid fetching it can be at the start of the loop, rather than at the end. But this has nothing to do with COVER_CLIP_BILINEAR and should not be part of what sets it. And it is true that there are a lot of broken bilinear fetches that do read the weight-zero pixel. These need to be fixed. But this still does not change the meaning of the flag. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] [PATCH 0/4] New fast paths and Raspberry Pi 1 benchmarking
On Thu, Aug 20, 2015 at 6:58 AM, Pekka Paalanen ppaala...@gmail.com wrote: A thing that explains a great deal of these anomalies, but not all of it, has something to do with function addresses. There are hypotheses that it might have to do with the branch predictor and its cache. We made a test targeting exactly that idea: pick a fast path function that seems to be most susceptible to unexpected changes, pad it with x nops before the function start and N-x nops after the function end. We never execute those nops, but changing x changes the function start address while keeping everything else in the whole binary in the same place. The results were mind-boggling: depending on the function starting address, the src__ L1 test of lowlevel-blt-bench went either 355 Mpx/s or 470 Mpx/s. There does not seem to be any predictable pattern on which addresses are fast and which are slow. Obviously this will screw up our benchmarks, because a change in an unrelated function may cause another function's address to shift, and therefore change its performance. See [1] for the plot. [1] The plot of alignment vs. performance https://git.collabora.com/cgit/user/pq/pixman-benchmarking.git/plain/octave/figures/fig-src---L1.pdf Could this be whether some bad instruction ends up next to or split by a cache line boundary? That would produce a random-looking plot, though it really is a plot of the location of the bad instructions in the measured function. If this really is a problem then the ideal fix is for the compiler to insert NOP instructions in order to move the bad instructions away from the locations that make them bad. Yike. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] RFC: Pixman benchmark CPU time measurement
On 06/03/2015 12:36 AM, Pekka Paalanen wrote: On Tue, 2 Jun 2015 15:03:01 -0700 Bill Spitzak spit...@gmail.com wrote: I would have the first call return 0.0 and all the others return the difference between current time and when that first call was done. Then there is no worry about accuracy of floating point. I do not think any callers are interested in the absolute time, only in subtracting two results to get an elapsed time. OpenMP threading must be taken into account, but that'd be doable. It would indeed allow removing the elapsed() function from my proposal. There would be a problem with clock() wrapping around too often, but that's an orthogonal issue. But it would only wrap 72 days after the test started, rather than at some random point during the test. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] RFC: Pixman benchmark CPU time measurement
I would have the first call return 0.0 and all the others return the difference between current time and when that first call was done. Then there is no worry about accuracy of floating point. I do not think any callers are interested in the absolute time, only in subtracting two results to get an elapsed time. Not sure if cpu time is what the benchmarks want. This does not include blocking waiting for the X server or for the GPU or for reading files. Elapsed real time is probably more useful. On Tue, Jun 2, 2015 at 9:03 AM, Ben Avison bavi...@riscosopen.org wrote: On Tue, 02 Jun 2015 08:32:35 +0100, Pekka Paalanen ppaala...@gmail.com wrote: most pixman performance benchmarks currently rely on gettime() from test/util.[ch]: - lowlevel-blt-bench - prng-test - radial-perf-test - scaling-bench Furthermore, affine-bench has its own gettimei() which is essentially gettime() but with uin32_t instead of double. For what it's worth, here's my opinion. I'll sidestep the issue of *which* underlying system clock is read for now, and look at data types. It certainly makes more sense to use doubles than floats for holding absolute times. As of 2005-09-05 05:58:26 UTC, the number of microseconds elapsed since 1970-01-01 00:00:00 UTC has been expressable as a 51-bit integer. The next time that changes will be 2041-05-10 11:56:53 UTC, when that goes up to a 52-bit integer. IEEE double-precision floating point numbers use a 52-bit mantissa, so they are capable of expressing all 51- and 52-bit integers without any loss of precision. In fact, we don't lose precision until we reach 54-bit integers (because the mantissa excludes the implicit leading '1' bit): after 2255-06-05 23:47:34 UTC the times would start being rounded to an even number of microseconds. With only 23 mantissa bits in single-precision, times would currently be rounded with a granularity of over 2 minutes - unworkable for most purposes. Even dividing by 1000, as gettime() does, is fairly harmless with double-precision floating point - all you're really doing is subtracting 20 from the exponent and adding a few multiples of the upper bits of the mantissa into the lower bits. But this is ignoring the fact that underneath we're calling gettimeofday(), which suffers from a perennial problem with clock APIs, the use of an absolute time expressed as an integer which is liable to overflow. There are a limited number of transformations you can safely perform on these - subtracting one from another is notable as a useful and safe operation (assuming the time interval is less than the maximum integer expressable, which will normally be the case). Assigning the time to a variable of wider type (such as assigning the long int tv_sec to a uint64_t) is *not* safe, unless you have a reference example of a nearby time that's already in the wider type, from which you can infer the most significant bits. There is no provision in the API as defined to pass in any such reference value, and when gettime() assigns the time to a double, that's effectively a very wide type indeed because it can hold the equivalent of an integer over 1000 bits long. Assuming 'long int' continues to be considered to be a signed 32-bit number, as it usually is for today's compilers, tv_sec will suffer signed overflow on 2038-01-19 03:14:08 UTC, which will hit long before we start losing precision for doubles. That's only 23 years away now, still within the careers of many of today's engineers. Dividing an integer absolute time is also no good, because differing values of the overflowed upper bits would completely scramble all the lower bits. gettimei() gets away with it in the #ifndef HAVE_GETTIMEOFDAY clause because CLOCKS_PER_SEC is normally 100 so the multiplication and division cancel each other out. Multiplication and addition, on the other hand, are OK so long as you don't widen the type because the missing upper bits only affect other missing upper bits in the result - hence why gettimei() multiplies tv_sec by 100 and adds tv_usec. The output of the function is safe to use to calculate time intervals so long as the interval doesn't exceed +/- 2^31 microseconds (about 35 minutes). If I were to make one change to gettimei() now, looking back, it would be to make it return int32_t instead. This is because most often you'd be subtracting two sample outputs of the function, and it's more often useful to consider time intervals as signed (say if you're comparing the current time against a timeout time which may be in the past or the future). If gettimei() returns a signed integer, then C's type promotion rules make the result of the subtraction signed automatically. Ben ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman ___ Pixman mailing list
Re: [Pixman] [PATCH pixman] test: Added more demos and tests to .gitignore file
On 05/04/2015 11:57 PM, Pekka Paalanen wrote: On Wed, 29 Apr 2015 11:44:17 -0700 Bill Spitzak spit...@gmail.com wrote: Uses a wildcard to handle the majority which end in -test. --- .gitignore | 45 + 1 file changed, 5 insertions(+), 40 deletions(-) Looks fine to me, so R-b me and pushed: e0c0153..6f14bae master - master .log and .trs files in test/ would probably be good to ignore too. They are created by 'make check'. I tried running make check but I don't get any such files and git status does not list any unknown files. However one test (stress-test) failed with a floating point exception for me. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman] test: Added more demos and tests to .gitignore file
Uses a wildcard to handle the majority which end in -test. --- .gitignore | 45 + 1 file changed, 5 insertions(+), 40 deletions(-) diff --git a/.gitignore b/.gitignore index 7da6b6f..a245b69 100644 --- a/.gitignore +++ b/.gitignore @@ -26,62 +26,27 @@ stamp-h? config.h config.h.in .*.swp -demos/alpha-test +demos/*-test demos/checkerboard demos/clip-in -demos/clip-test -demos/composite-test -demos/conical-test -demos/convolution-test -demos/gradient-test demos/linear-gradient demos/quad2quad -demos/radial-test demos/scale -demos/screen-test -demos/srgb-test -demos/srgb-trap-test -demos/trap-test -demos/tri-test pixman/pixman-srgb.c pixman/pixman-version.h -test/a1-trap-test +test/*-test test/affine-bench -test/affine-test test/alpha-loop test/alphamap -test/alpha-test -test/blitters-test +test/check-formats test/clip-in -test/clip-test -test/combiner-test test/composite -test/composite-test -test/composite-traps-test -test/convolution-test -test/fetch-test -test/glyph-test -test/gradient-crash-test -test/gradient-test test/infinite-loop test/lowlevel-blt-bench -test/oob-test -test/pdf-op-test -test/prng-test -test/radial-perf-test -test/region-contains-test -test/region-test +test/radial-invalid test/region-translate -test/region-translate-test -test/rotate-test -test/scaling-crash-test -test/scaling-helpers-test -test/scaling-test -test/screen-test -test/stress-test +test/scaling-bench test/trap-crasher -test/trap-test -test/window-test *.pdb *.dll *.lib -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 07/15] pixman-filter: Speed up the BOX+BOX filter
This is easy as the caller already intersected the two boxes, so the width is the integral. --- pixman/pixman-filter.c |5 + 1 file changed, 5 insertions(+) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 4aafa51..782f73d 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -182,6 +182,11 @@ integral (pixman_kernel_t reconstruct, double x1, assert (width == 0.0); return filters[sample].func (x2 / scale); } +else if (reconstruct == PIXMAN_KERNEL_BOX sample == PIXMAN_KERNEL_BOX) +{ + assert (width = 1.0); + return width; +} else if (sample == PIXMAN_KERNEL_IMPULSE) { assert (width == 0.0); -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 13/15] pixman-filter: refactor cubic polynominal and don't range check
The other filters do not check for x being in range, so there is no reason for cubic to do so. --- pixman/pixman-filter.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 7e10108..bf9dce3 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -109,18 +109,16 @@ general_cubic (double x, double B, double C) if (ax 1) { - return ((12 - 9 * B - 6 * C) * ax * ax * ax + - (-18 + 12 * B + 6 * C) * ax * ax + (6 - 2 * B)) / 6; -} -else if (ax = 1 ax 2) -{ - return ((-B - 6 * C) * ax * ax * ax + - (6 * B + 30 * C) * ax * ax + (-12 * B - 48 * C) * - ax + (8 * B + 24 * C)) / 6; + return (((12 - 9 * B - 6 * C) * ax + +(-18 + 12 * B + 6 * C)) * ax * ax + + (6 - 2 * B)) / 6; } else { - return 0; + return -B - 6 * C) * ax + +(6 * B + 30 * C)) * ax + + (-12 * B - 48 * C)) * ax + + (8 * B + 24 * C)) / 6; } } -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 01/15] demos/scale: Compute filter size using boundary of xformed ellipse, not rectangle
This is much more accurate and less blurry. In particular the filtering does not change as the image is rotated. --- demos/scale.c | 43 ++- 1 file changed, 2 insertions(+), 41 deletions(-) diff --git a/demos/scale.c b/demos/scale.c index d00307e..71c7791 100644 --- a/demos/scale.c +++ b/demos/scale.c @@ -55,50 +55,11 @@ get_widget (app_t *app, const char *name) return widget; } -static double -min4 (double a, double b, double c, double d) -{ -double m1, m2; - -m1 = MIN (a, b); -m2 = MIN (c, d); -return MIN (m1, m2); -} - -static double -max4 (double a, double b, double c, double d) -{ -double m1, m2; - -m1 = MAX (a, b); -m2 = MAX (c, d); -return MAX (m1, m2); -} - static void compute_extents (pixman_f_transform_t *trans, double *sx, double *sy) { -double min_x, max_x, min_y, max_y; -pixman_f_vector_t v[4] = -{ - { { 1, 1, 1 } }, - { { -1, 1, 1 } }, - { { -1, -1, 1 } }, - { { 1, -1, 1 } }, -}; - -pixman_f_transform_point (trans, v[0]); -pixman_f_transform_point (trans, v[1]); -pixman_f_transform_point (trans, v[2]); -pixman_f_transform_point (trans, v[3]); - -min_x = min4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); -max_x = max4 (v[0].v[0], v[1].v[0], v[2].v[0], v[3].v[0]); -min_y = min4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); -max_y = max4 (v[0].v[1], v[1].v[1], v[2].v[1], v[3].v[1]); - -*sx = (max_x - min_x) / 2.0; -*sy = (max_y - min_y) / 2.0; +*sx = hypot (trans-m[0][0], trans-m[0][1]) / trans-m[2][2]; +*sy = hypot (trans-m[1][0], trans-m[1][1]) / trans-m[2][2]; } typedef struct -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 11/15] pixman-filter: made IMPULSE.IMPULSE not produce a zero-wide filter
With the other patch to put error on the center pixel, this produces the same result as BOX.IMPULSE filter. --- pixman/pixman-filter.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 00126cd..64981cd 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -327,7 +327,9 @@ pixman_filter_create_separable_convolution (int *n_values, subsample_y = (1 subsample_bits_y); width = filter_width (reconstruct_x, sample_x, sx); +if (width 1) width = 1; height = filter_width (reconstruct_y, sample_y, sy); +if (height 1) height = 1; *n_values = 4 + width * subsample_x + height * subsample_y; -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 14/15] pixman-filter: Gaussian fixes
The SIGMA term drops out on simplification. Expanded the size slightly (from ~4.25 to 5) to make the cutoff less noticable. The filter is truncated at a value of .001 instead of .006, this new value is less than 1/2 of 1/255, rather than greater than it. --- pixman/pixman-filter.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index bf9dce3..7d7b381 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -63,10 +63,7 @@ linear_kernel (double x) static double gaussian_kernel (double x) { -#define SQRT2 (1.4142135623730950488016887242096980785696718753769480) -#define SIGMA (SQRT2 / 2.0) - -return exp (- x * x / (2 * SIGMA * SIGMA)) / (SIGMA * sqrt (2.0 * M_PI)); +return exp (- x * x) / sqrt (M_PI); } static double @@ -139,7 +136,7 @@ static const filter_info_t filters[] = { PIXMAN_KERNEL_BOX, box_kernel, 1.0 }, { PIXMAN_KERNEL_LINEAR,linear_kernel,2.0 }, { PIXMAN_KERNEL_CUBIC, cubic_kernel, 4.0 }, -{ PIXMAN_KERNEL_GAUSSIAN, gaussian_kernel, 6 * SIGMA }, +{ PIXMAN_KERNEL_GAUSSIAN, gaussian_kernel, 5.0 }, { PIXMAN_KERNEL_LANCZOS2, lanczos2_kernel, 4.0 }, { PIXMAN_KERNEL_LANCZOS3, lanczos3_kernel, 6.0 }, { PIXMAN_KERNEL_LANCZOS3_STRETCHED, nice_kernel, 8.0 }, -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 02/15] demos/scale: Added pulldown to choose PIXMAN_FILTER_* value
This allows testing of GOOD/BEST and to do comparisons between the basic filters and PIXMAN_FILTER_SEPARABLE_CONVOLUTION settings. --- demos/scale.c | 14 +- demos/scale.ui | 40 ++-- 2 files changed, 43 insertions(+), 11 deletions(-) diff --git a/demos/scale.c b/demos/scale.c index 71c7791..203168f 100644 --- a/demos/scale.c +++ b/demos/scale.c @@ -68,6 +68,15 @@ typedef struct intvalue; } named_int_t; +static const named_int_t filter_types[] = +{ +{ Separable, PIXMAN_FILTER_SEPARABLE_CONVOLUTION }, +{ Nearest, PIXMAN_FILTER_NEAREST }, +{ Bilinear, PIXMAN_FILTER_BILINEAR }, +{ Good, PIXMAN_FILTER_GOOD }, +{ Best, PIXMAN_FILTER_BEST }, +}; + static const named_int_t filters[] = { { Box, PIXMAN_KERNEL_BOX }, @@ -201,7 +210,9 @@ rescale (GtkWidget *may_be_null, app_t *app) gtk_adjustment_get_value (app-subsample_adjustment), gtk_adjustment_get_value (app-subsample_adjustment)); -pixman_image_set_filter (app-original, PIXMAN_FILTER_SEPARABLE_CONVOLUTION, params, n_params); +pixman_image_set_filter (app-original, + get_value (app, filter_types, filter_combo_box), + params, n_params); pixman_image_set_repeat ( app-original, get_value (app, repeats, repeat_combo_box)); @@ -343,6 +354,7 @@ app_new (pixman_image_t *original) widget = get_widget (app, drawing_area); g_signal_connect (widget, expose_event, G_CALLBACK (on_expose), app); +set_up_combo_box (app, filter_combo_box, G_N_ELEMENTS (filter_types), filter_types); set_up_filter_box (app, reconstruct_x_combo_box); set_up_filter_box (app, reconstruct_y_combo_box); set_up_filter_box (app, sample_x_combo_box); diff --git a/demos/scale.ui b/demos/scale.ui index ee985dd..b62cbfb 100644 --- a/demos/scale.ui +++ b/demos/scale.ui @@ -191,12 +191,23 @@ property name=column_spacing8/property property name=row_spacing6/property child + object class=GtkLabel id=labelF +property name=visibleTrue/property +property name=xalign1/property +property name=label translatable=yeslt;bgt;Filter:lt;/bgt;/property +property name=use_markupTrue/property + /object +/child +child object class=GtkLabel id=label4 property name=visibleTrue/property property name=xalign1/property property name=label translatable=yeslt;bgt;Reconstruct X:lt;/bgt;/property property name=use_markupTrue/property /object + packing +property name=top_attach1/property + /packing /child child object class=GtkLabel id=label5 @@ -206,7 +217,7 @@ property name=use_markupTrue/property /object packing -property name=top_attach1/property +property name=top_attach2/property /packing /child child @@ -217,7 +228,7 @@ property name=use_markupTrue/property /object packing -property name=top_attach2/property +property name=top_attach3/property /packing /child child @@ -228,7 +239,7 @@ property name=use_markupTrue/property /object packing -property name=top_attach3/property +property name=top_attach4/property /packing /child child @@ -239,7 +250,7 @@ property name=use_markupTrue/property /object packing -property name=top_attach4/property +property name=top_attach5/property /packing /child child @@ -250,7 +261,15 @@ property name=use_markupTrue/property /object packing -property name=top_attach5/property +property name=top_attach6/property + /packing +/child +child + object class=GtkComboBox id=filter_combo_box +
[Pixman] [PATCH pixman 12/15] pixman-filter: Turn off subsampling when not necessary
If sample is IMPULSE and reconstruct is BOX or IMPULSE the sub-pixel position of the sample is not relevant, so only one subsample is needed. --- pixman/pixman-filter.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 64981cd..7e10108 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -230,6 +230,8 @@ filter_width (pixman_kernel_t reconstruct, pixman_kernel_t sample, double scale) { +if (reconstruct == PIXMAN_KERNEL_BOX sample == PIXMAN_KERNEL_IMPULSE) + return 0; return ceil (scale * filters[sample].width + filters[reconstruct].width); } @@ -323,13 +325,13 @@ pixman_filter_create_separable_convolution (int *n_values, int subsample_x, subsample_y; int width, height; -subsample_x = (1 subsample_bits_x); -subsample_y = (1 subsample_bits_y); - width = filter_width (reconstruct_x, sample_x, sx); -if (width 1) width = 1; +if (width 1) { width = 1; subsample_bits_x = 0; } height = filter_width (reconstruct_y, sample_y, sy); -if (height 1) height = 1; +if (height 1) { height = 1; subsample_bits_y = 0; } + +subsample_x = (1 subsample_bits_x); +subsample_y = (1 subsample_bits_y); *n_values = 4 + width * subsample_x + height * subsample_y; -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 05/15] pixman-filter: Correct Simpsons integration
Simpsons uses cubic curve fitting, with 3 samples defining each cubic. This makes the weights of the samples be in a pattern of 1,4,2,4,2...4,1, and then dividing the result by 3. The previous code was using weights of 1,2,6,6...6,2,1 which produced about 2x the correct value, as it was still dividing by 3. The filter normalization removed this error. Also this is effectively a linear interpolation except for the ends. --- pixman/pixman-filter.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 15f9069..7c1da0d 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -204,11 +204,14 @@ integral (pixman_kernel_t reconstruct, double x1, { double a1 = x1 + h * i; double a2 = x2 + h * i; + s += 4 * SAMPLE(a1, a2); + } - s += 2 * SAMPLE (a1, a2); - - if (i = 2 i N_SEGMENTS - 1) - s += 4 * SAMPLE (a1, a2); + for (i = 2; i N_SEGMENTS; i += 2) + { + double a1 = x1 + h * i; + double a2 = x2 + h * i; + s += 2 * SAMPLE(a1, a2); } s += SAMPLE (x1 + width, x2 + width); -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
Re: [Pixman] Is Pixman being maintained at all?
On 04/07/2015 12:19 PM, Matt Turner wrote: I don't know how Cairo does review, but I think it would be really nice if a Cairo developer reviewed Bill's patches (I think they were adding a new API to pixman?) if not for all the little technical details but that the API makes sense for its uses in Cairo. My patches are to move the current cairo FILTER_GOOD/BEST behavior into pixman. Pixman can do this better and faster, and it means that the X11 Cairo backend does not have to do an image fallback for transforms. The main difference between this patch and the Cairo code is that Søren wanted to preserve his current api for generating filters, which requires selection of two filters which are then integrated (one of them scaled by the width). I am not in full agreement with this as all other systems I have ever seen treat the smaller filter as impulse, and except for box+box (which produces a trapezoid and most software produces that directly when box is requested), I have to use impulse for one of the filters always. In any case most of the series is to fix bugs and inefficiencies in the filter generation. There is then a patch to enable use of the filtering for GOOD/BEST settings, matching Cairo (with a few minor improvements). There is also patches to fix the demo programs so the GOOD/BEST filter can be tested and to fix the filter size for rotated images. In addition if this is pushed we need a test in Cairo to detect if pixman is new enough and then remove Cairo's local emulation. There has to be a test for the linked Pixman and perhaps a different one for the X server. ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 14/15] pixman-filter: Gaussian fixes
The SIGMA term drops out on simplification. Expanded the size slightly (from ~4.25 to 5) to make the cutoff less noticable. The filter is truncated at a value of .001 instead of .006, this new value is less than 1/2 of 1/255, rather than greater than it. --- pixman/pixman-filter.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index bf9dce3..7d7b381 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -63,10 +63,7 @@ linear_kernel (double x) static double gaussian_kernel (double x) { -#define SQRT2 (1.4142135623730950488016887242096980785696718753769480) -#define SIGMA (SQRT2 / 2.0) - -return exp (- x * x / (2 * SIGMA * SIGMA)) / (SIGMA * sqrt (2.0 * M_PI)); +return exp (- x * x) / sqrt (M_PI); } static double @@ -139,7 +136,7 @@ static const filter_info_t filters[] = { PIXMAN_KERNEL_BOX, box_kernel, 1.0 }, { PIXMAN_KERNEL_LINEAR,linear_kernel,2.0 }, { PIXMAN_KERNEL_CUBIC, cubic_kernel, 4.0 }, -{ PIXMAN_KERNEL_GAUSSIAN, gaussian_kernel, 6 * SIGMA }, +{ PIXMAN_KERNEL_GAUSSIAN, gaussian_kernel, 5.0 }, { PIXMAN_KERNEL_LANCZOS2, lanczos2_kernel, 4.0 }, { PIXMAN_KERNEL_LANCZOS3, lanczos3_kernel, 6.0 }, { PIXMAN_KERNEL_LANCZOS3_STRETCHED, nice_kernel, 8.0 }, -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 07/15] pixman-filter: Speed up the BOX+BOX filter
This is easy as the caller already intersected the two boxes, so the width is the integral. --- pixman/pixman-filter.c |5 + 1 file changed, 5 insertions(+) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 4aafa51..782f73d 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -182,6 +182,11 @@ integral (pixman_kernel_t reconstruct, double x1, assert (width == 0.0); return filters[sample].func (x2 / scale); } +else if (reconstruct == PIXMAN_KERNEL_BOX sample == PIXMAN_KERNEL_BOX) +{ + assert (width = 1.0); + return width; +} else if (sample == PIXMAN_KERNEL_IMPULSE) { assert (width == 0.0); -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 12/15] pixman-filter: Turn off subsampling when not necessary
If sample is IMPULSE and reconstruct is BOX or IMPULSE the sub-pixel position of the sample is not relevant, so only one subsample is needed. --- pixman/pixman-filter.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index 64981cd..7e10108 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -230,6 +230,8 @@ filter_width (pixman_kernel_t reconstruct, pixman_kernel_t sample, double scale) { +if (reconstruct == PIXMAN_KERNEL_BOX sample == PIXMAN_KERNEL_IMPULSE) + return 0; return ceil (scale * filters[sample].width + filters[reconstruct].width); } @@ -323,13 +325,13 @@ pixman_filter_create_separable_convolution (int *n_values, int subsample_x, subsample_y; int width, height; -subsample_x = (1 subsample_bits_x); -subsample_y = (1 subsample_bits_y); - width = filter_width (reconstruct_x, sample_x, sx); -if (width 1) width = 1; +if (width 1) { width = 1; subsample_bits_x = 0; } height = filter_width (reconstruct_y, sample_y, sy); -if (height 1) height = 1; +if (height 1) { height = 1; subsample_bits_y = 0; } + +subsample_x = (1 subsample_bits_x); +subsample_y = (1 subsample_bits_y); *n_values = 4 + width * subsample_x + height * subsample_y; -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
[Pixman] [PATCH pixman 10/15] pixman-filter: don't range-check impulse filter
The other filters don't range-check, so there is no need for this one to either. It is only called with x==0. --- pixman/pixman-filter.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixman/pixman-filter.c b/pixman/pixman-filter.c index fbc657d..00126cd 100644 --- a/pixman/pixman-filter.c +++ b/pixman/pixman-filter.c @@ -45,7 +45,7 @@ typedef struct static double impulse_kernel (double x) { -return (x == 0.0)? 1.0 : 0.0; +return 1; } static double -- 1.7.9.5 ___ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman