Re: [Pixman] [PATCH 10/15] pixman-filter: don't range-check impulse filter

[Oded Gabbay]

On Tue, Dec 22, 2015 at 10:51 PM, Bill Spitzak  wrote:
> 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.
>

So my response here is at the same I wrote about patch 7/15 (defer
until cairo will actually use this function), only here we are talking
about showing results of cairo test suite and not benchmarking.

Thanks,

   Oded

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

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

[Pixman] [PATCH 10/15] pixman-filter: don't range-check impulse filter

[spitzak]

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