Re: [Pixman] Dithering patches, v2

[Bill Spitzak]

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

[Bill Spitzak]

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?

[Bill Spitzak]

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.

[Bill Spitzak]

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.

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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 Sandmann 
wrote:

> 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

[Bill Spitzak]

On Fri, Apr 29, 2016 at 4:55 AM, Oded Gabbay  wrote:

> 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)

[Bill Spitzak]

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)

[Bill Spitzak]

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)

[Bill Spitzak]

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íček 
wrote:

> 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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

On Tue, Apr 12, 2016 at 1:55 PM, Søren Sandmann 
wrote:

>
>   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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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 Sandmann 
wrote:

>
> 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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

Great idea to test this.

On Mon, Mar 14, 2016 at 11:24 PM, Søren Sandmann 
wrote:

> 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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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 ()

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

On Mon, Feb 1, 2016 at 6:10 AM, Oded Gabbay  wrote:

>
> > -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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

On Thu, Jan 7, 2016 at 11:44 PM, Pekka Paalanen  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.
___
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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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.

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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 Gabbay  wrote:

> 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’

[Bill Spitzak]

On Mon, Sep 21, 2015 at 10:07 PM, Søren Sandmann 
wrote:

> 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

[Bill Spitzak]

On Wed, Sep 16, 2015 at 7:30 AM, Ben Avison  wrote:

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

[Bill Spitzak]

On Mon, Sep 14, 2015 at 11:52 AM, Søren Sandmann 
wrote:

>
> 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

[Bill Spitzak]

On Thu, Sep 10, 2015 at 9:35 AM, Ben Avison  wrote:

> 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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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?

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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

[Bill Spitzak]

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