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

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
> From: Bill Spitzak 
>>
>> 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 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
>>
>
> 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 environment variable
> is not supposed to change Pixman's behavior, only potentially its
> performance.
>

Will look into that. I would like the rest of the detail please.

Currently Linux 2D rendering is a joke. Nobody was using the
SeparateConvolution until I patched up Cairo to do it, but I sure figured
that was a very temporary patch until it could be done in a proper
location. Users want "good", not "low level api". So now Linux is 1.5 years
further behind. 

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

> On Tue, Feb 16, 2016 at 4:45 PM, Bill Spitzak  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 
wrote:

>
>
> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> v11: Restored range checks
>>
>> Signed-off-by: Bill Spitzak 
>> Reviewed-by: Oded Gabbay 
>>
>
> 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 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: 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. 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 
wrote:

>
>
> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
>> Reviewed-by: Oded Gabbay 
>>
>
> 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 
>

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

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
wrote:

>
>
> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
>> Reviewed-by: Oded Gabbay 
>>
>
> 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 
wrote:

> On Wed, Feb 10, 2016 at 1:25 AM,  wrote:
>
>> From: Bill Spitzak 
>>
>> 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 
>> Reviewed-by: Oded Gabbay 
>>
>
>
> 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 14/14] demos/scale: default to GOOD and locked-together axis

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

From: Bill Spitzak 
>
> 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.


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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 
>

The short answer to this patch is NACK - it doesn't make sense to change
the meaning of GOOD and BEST.

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.

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.

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.

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 environment variable
is not supposed to change Pixman's behavior, only potentially its
performance.


Søren
___
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

[Søren Sandmann]

On Tue, Feb 16, 2016 at 4:45 PM, Bill Spitzak  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.


Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v13 10/14] pixman-filter: Gaussian fixes

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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.

Expanding the size is fine with me.


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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.

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.

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


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> v11: Restored range checks
>
> Signed-off-by: Bill Spitzak 
> Reviewed-by: Oded Gabbay 
>

What's the point of this? All this does is putting ax^2 outside of the
parenthesis?


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: 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. 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.

There should be spaces before the parentheses in the SAMPLE() macros,
though.


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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 
> Reviewed-by: Oded Gabbay 
>


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


Søren
___
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

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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 
> Reviewed-by: Oded Gabbay 
>

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 


Søren
___
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 ()

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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.


Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH v13 04/14] pixman-image: Added enable-gnuplot config to view filters in gnuplot

[Søren Sandmann]

On Wed, Feb 10, 2016 at 1:25 AM,  wrote:

> From: Bill Spitzak 
>
> 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
>
> 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 
> Reviewed-by: Oded Gabbay 
>

This is an interesting idea. It may be worth having it compiled in at all
times, but disabled unless a PIXMAN_GNUPLOT environment variable is set.

There is one formatting issue with this patch: Pixman coding style requires
a space before parentheses in function calls.


Søren
___
Pixman mailing list
Pixman@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pixman