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

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: 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.
> 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: 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;
>
> 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  wrote:

> On Tue, Dec 22, 2015 at 9:44 PM, Bill Spitzak  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 
> wrote:
> >>
> >> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> >> > From: 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.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  wrote:

> On Tue, Dec 22, 2015 at 9:25 PM, Bill Spitzak  wrote:
> >
> >
> > On Tue, Dec 22, 2015 at 2:32 AM, Oded Gabbay 
> wrote:
> >>
> >> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> >> > From: 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.
> >>
> >> 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: 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.
>
> 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: Bill Spitzak 
> >
> > 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 07/15] pixman-filter: Speed up the BOX+BOX filter

[Oded Gabbay]

On Tue, Dec 22, 2015 at 9:44 PM, Bill Spitzak  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  wrote:
>>
>> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
>> > From: 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.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 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: Bill Spitzak 
> >
> > 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: Bill Spitzak 
> >
> > 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  wrote:

> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> > From: 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.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

[Oded Gabbay]

On Tue, Dec 22, 2015 at 9:25 PM, Bill Spitzak  wrote:
>
>
> On Tue, Dec 22, 2015 at 2:32 AM, Oded Gabbay  wrote:
>>
>> On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
>> > From: 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.
>>
>> 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 07/15] pixman-filter: Speed up the BOX+BOX filter

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: 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.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 08/15] pixman-filter: Don't recurse unnecessarily.

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: Bill Spitzak 
>
> 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 ?

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 10/15] pixman-filter: don't range-check impulse filter

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: 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.

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 ?

   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 09/15] pixman-filter: put filter error on center pixel

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: Bill Spitzak 
>
> 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 11/15] pixman-filter: made IMPULSE.IMPULSE not produce a zero-wide filter

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: 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;

Please put the assignment in a new line

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

   Oded
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Cleaning patchwork

[Oded Gabbay]

Hi Ben,

I would like to clean pixman's patchwork and you have about 20
outstanding patches.

Could you please take a look and tell me which ones are still relevant
and which ones are not ?

You can take a look at http://patchwork.freedesktop.org/project/pixman/patches/
I also copied here there names:

[4/4] pixman-fast-path: Make bilinear cover fetcher use COVER_CLIP_TIGHT flag
[3/4] armv7/mips/sse2: Make bilinear cover fast paths use COVER_CLIP_TIGHT flag
[2/4] Introduce FAST_PATH_SAMPLES_COVER_CLIP_TIGHT_BILINEAR flag
Resolve implementation-defined behaviour for division rounded to -infinity
[24/37,v2] armv6: Add optimised scanline fetcher for a1r5g5b5
[23/37,v2] armv6: Add optimised scanline fetchers and writeback for
r5g6b5 and a8
Require destination images to be bitmaps
[2/5,v2] armv7: Add in__8 fast path
[11/37,v2] armv6: Add in__8 fast path
[10/37,v2] pixman-fast-path: Add in__8 fast path
[05/37,v2] composite flags: Early detection of fully opaque 1x1
repeating source images
[5/5] armv7: Add src_1555_ fast path
[4/5] armv7: Add in_reverse__ fast path
[3/5] armv7: Add in_n_ fast path
[1/5] armv7: Use prefetch for small-width images too
armv7: Re-use existing fast paths in more cases
armv7: Re-use existing fast paths in more cases
[25/37,v2] armv6: Add src_1555_ fast path
[3/3] armv7: Use VLD-to-all-lanes
[2/3] armv7: Faster fill operations
[1/3] armv7: Coalesce scalar accesses where possible

I also suggest that for the relevant ones (if there are any), you
would rebase them on the latest master and re-send them as a single
series in order to restart the review process.

Thanks,

Oded
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] Cleaning patchwork - Nemanja

[Oded Gabbay]

Hi Nemanja,

I would like to clean pixman's patchwork and you have about 10
outstanding patches.

Could you please take a look and tell me which ones are still relevant
and which ones are not ?

You can take a look at http://patchwork.freedesktop.org/project/pixman/patches/
I also copied here there names:

[13/13] MIPS: enable prefetch for store only for CPU with 32 byte cache line
[12/13] MIPS: disabled non 32-bit platforms
[11/13] MIPS: mips32r2: Added optimization for function pixman_fill_buff16
[10/13] MIPS: dspr1: Move fast paths implementation from dspr2 to dspr1
[09/13] MIPS: dspr1: empty implementation with runtime detection
[08/13] MIPS: mips32r2: Move fast paths implementation from dspr2 to mips32r2
[07/13] MIPS: mips32r2: empty implementation with runtime detection
[06/13] MIPS: dspr2: runtime detection extended
[05/13] Implementing memcpy through pointer
[03/13] MIPS: dspr2: Removed build restrictions and repair compiler's check

I also suggest that for the relevant ones (if there are any), you
would rebase them on the latest master and re-send them as a single
series in order to restart the review process.

Thanks,

Oded
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Allow building on Windows with cmd.exe

[Oded Gabbay]

On Thu, Jun 4, 2015 at 12:02 PM, Andrea Canciani  wrote:
> The patch does not regress the mingw-based build (basically that used in
> http://cairographics.org/end_to_end_build_for_win32/ )
> I did not manage to get the build working on the cmd.exe shell. Do you have
> any instructions (or possibly indications about what is the correct
> environment) for that?
>
> AFAICT the change still relies on a Unix-like make command.
> A bigger (but possibly even more interesting) change would be an NMAKE-based
> build, so that the command-line tools available in Visual Studio are
> sufficient for building the whole project.
>
> Andrea
>

Hi Andrea,

I know it has been almost 6 months since you last looked at this
patch, but I would like to ask if you can give it an r-b and then I
will push it to master.

If not, what needs to be done to give it an r-b ?

Unfortunately, I don't have a Windows machine to check this.

Thanks,

Oded

>
> On Mon, Jun 1, 2015 at 8:44 AM, Andrea Canciani  wrote:
>>
>> The ping was good and the changes make sense. Currently I am replying from
>> the phone and I will not have access to my computer until the 3rd, but I am
>> flagging this mail so that I will test and review the patch asap.
>>
>> Sorry for the delay
>> Andrea
>>
>> On Jun 1, 2015 8:09 AM, "Siarhei Siamashka" 
>> wrote:
>>>
>>> On Wed, 22 Apr 2015 04:01:31 +0200
>>> Simon Richter  wrote:
>>>
>>> > This finds out whether the standard shell for make is cmd.exe, and
>>> > adjusts
>>> > the build process accordingly.
>>> > ---
>>> >  Makefile.win32.common | 14 --
>>> >  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> Thanks for the patch.
>>>
>>> But it is a bit difficult for non-Windows folks to see if
>>> it makes any sense or not. So it would be best if some other
>>> Windows user could review and confirm that it works.
>>>
>>> I have added Andrea Canciani (the author of the win32 makefiles)
>>> to CC.
>>>
>>> > diff --git a/Makefile.win32.common b/Makefile.win32.common
>>> > index 777f94c..335886f 100644
>>> > --- a/Makefile.win32.common
>>> > +++ b/Makefile.win32.common
>>> > @@ -1,5 +1,15 @@
>>> >  LIBRARY = pixman-1
>>> >
>>> > +ifeq ($(shell echo ""),)
>>> > +# POSIX style shell
>>> > +mkdir_p = mkdir -p $1
>>> > +rm = $(RM) $1
>>> > +else
>>> > +# DOS/Windows style shell
>>> > +mkdir_p = if not exist $(subst /,\,$1) md $(subst /,\,$1)
>>> > +rm = del $(subst /,\,$1)
>>> > +endif
>>> > +
>>> >  CC = cl
>>> >  LD = link
>>> >  AR = lib
>>> > @@ -47,10 +57,10 @@ endif
>>> >
>>> >
>>> >  $(CFG_VAR)/%.obj: %.c $(libpixman_headers)
>>> > - @mkdir -p $(CFG_VAR)
>>> > + @$(call mkdir_p,$(@D))
>>> >   @$(CC) -c $(PIXMAN_CFLAGS) -Fo"$@" $<
>>> >
>>> >  clean: inform
>>> > - @$(RM) $(CFG_VAR)/*.{exe,ilk,lib,obj,pdb} $(BUILT_SOURCES) ||
>>> > exit 0
>>> > + @-$(call rm,$(CFG_VAR)/*.exe $(CFG_VAR)/*.ilk $(CFG_VAR)/*.lib
>>> > $(CFG_VAR)/*.obj $(CFG_VAR)/*.pdb} $(BUILT_SOURCES))
>>> >
>>> >  .PHONY: inform clean
>>>
>>>
>>>
>>> --
>>> Best regards,
>>> Siarhei Siamashka
>
>
>
> ___
> 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

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: Bill Spitzak 
>
> 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 ?

   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

[Oded Gabbay]

On Sat, Dec 12, 2015 at 8:06 PM,   wrote:
> From: 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.
Why ?
I mean why it is not relevant ? and why only one subsample is 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

[Pixman] [ANNOUNCE] pixman release 0.33.6 now available

[Oded Gabbay]

A new pixman development release 0.33.6 is now available. This is the final 
release candidate leading up to the 0.34 stable release. The stable release is 
expected to be announced at the end of January 2016. 

From this point forward, only bug fixes can be accepted to the 0.34 release.

This release contains two minor fixes over the previous release (0.33.4):

- Fix detection of "K" constraint
- Fix detection of SSE2 & SSSE3 assembler support

See the git log below for full list of commits.

As usual, I will update the Fedora packages for F23 and rawhide in the next 
couple of days.

Thanks,

 Oded


tar.gz:
http://cairographics.org/snapshots/pixman-0.33.6.tar.gz
http://xorg.freedesktop.org/archive/individual/lib/pixman-0.33.6.tar.gz

tar.bz2:
http://xorg.freedesktop.org/archive/individual/lib/pixman-0.33.6.tar.bz2

Hashes:
MD5:  3abaa422e498b6f10ca69548ca383730  pixman-0.33.6.tar.gz
MD5:  e903e990f906bf76166717d6a6582950  pixman-0.33.6.tar.bz2
SHA1: f174d00517e7e1d81c90c65efc20dd876877d904  pixman-0.33.6.tar.gz
SHA1: 11e93fed35deb9c89347e7a7da3060e5e5c89412  pixman-0.33.6.tar.bz2

GPG signature:
http://cairographics.org/snapshots/pixman-0.33.6.tar.gz.sha1.asc
(signed by Oded Gabbay )

Git:
git://git.freedesktop.org/git/pixman
tag: pixman-0.33.6

Log:
Andrea Canciani (1):
  mmx: Improve detection of support for "K" constraint

Matt Turner (1):
  Revert "mmx: Use MMX2 intrinsics from xmmintrin.h directly."

Oded Gabbay (3):
  Post-release version bump to 0.33.5
  configura.ac: fix test for SSE2 & SSSE3 assembler support
  Pre-release version bump to 0.33.6
___
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

[Oded Gabbay]

On Wed, Dec 9, 2015 at 10: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

So it got delayed by 1 week, but I just released the 0.33.6 version.
I created a new branch - 0.34 - and only bug fixes should be put into
that branch from now on.

Regular development can continue to be done on master, as usual.

As a reminder, I'm planning to do the 0.34.0 release sometime near the
end of January.

Thanks,

   Oded
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman