[Pixman] [PATCH v13 13/14] pixman-image: Implement PIXMAN_FILTER_GOOD/BEST as separable convolutions

[Pekka Paalanen]

On Fri, 4 Mar 2016 19:12:36 -0800
Bill Spitzak <spitzak at gmail.com> wrote:

> On Fri, Mar 4, 2016 at 4:17 PM, Søren Sandmann <soren.sandmann at gmail.com>
> wrote:
> 
> > On Wed, Feb 10, 2016 at 1:25 AM, <spitzak at gmail.com> wrote:
> >  
> >> From: Bill Spitzak <spitzak at gmail.com>
> >>
> >> Detects and uses PIXMAN_FILTER_NEAREST for all 8 90-degree rotations and
> >> reflections when the scale is 1.0 and integer translation.
> >>
> >> GOOD uses:
> >>
> >>  scale < 1/16 : BOX.BOX at size 16
> >>  scale < 3/4 : BOX.BOX at size 1/scale
> >>  larger : BOX.BOX at size 1
> >>
> >>  If both directions have a scale >= 3/4 or a scale of 1/2 and an integer
> >>  translation, the faster PIXMAN_FILTER_BILINEAR code is used. This is
> >>  compatable at these scales with older versions of pixman where bilinear
> >>  was always used for GOOD.
> >>
> >> BEST uses:
> >>
> >>  scale < 1/24 : BOX.BOX at size 24
> >>  scale < 1/16 : BOX.BOX at size 1/scale
> >>  scale < 1 : IMPULSE.LANCZOS2 at size 1/scale
> >>  scale < 2.333 : IMPULSE.LANCZOS2 at size 1
> >>  scale < 128 : BOX.LANCZOS2 at size 1/(scale-1) (antialiased square
> >> pixels)
> >>  larger : BOX.LANCZOS2 at size 1/127 (antialias blur gets thicker)
> >>
> >> v8: Cutoff in BEST between IMPULSE.LANCZOS2 and BOX.LANCZOS2 adjusted for
> >>     a better match between the filters.
> >>
> >> v9: Use the new negative subsample controls to scale the subsamples. These
> >>     were chosen by finding the lowest number that did not add visible
> >>     artifacts to the zone plate image.
> >>
> >>     Scale demo altered to default to GOOD and locked-together x+y scale
> >>
> >>     Fixed divide-by-zero from all-zero matrix found by stress-test
> >>
> >> v11: Whitespace and formatting fixes
> >>      Moved demo changes to a later patch
> >>
> >> v12: Whitespace and formatting fixes
> >>
> >> Signed-off-by: Bill Spitzak <spitzak at gmail.com>
> >>  
> >
> > The short answer to this patch is NACK - it doesn't make sense to change
> > the meaning of GOOD and BEST.
> >  
> 
> Okay that is going to need a big WTF?

No, it's perfectly understandable.

> > There are at least three reasons:
> >
> > 1. Pixman is a low-level API that is in the camp of "do what I say" not
> > "do what I mean". When users such as cairo specify GOOD and BEST, they are
> > essentially asking Pixman to make a tradeoff that it doesn't have the
> > knowledge to do. As such, I think it was a mistake to have these values in
> > the first place (and indeed also a mistake to have them in the Render
> > extension since X11 is also supposed to be a do-what-I-say API). As such, I
> > think they should remain aliases to BILINEAR as they are now.
> >  
> 
> I have no idea why having three ways to say BILINEAR is necessary to "do
> what I say". One seems sufficient.

You must forget any resemblance the strings "GOOD" and "BEST" have
with English, and see them as abstract names for particular
mathematical functions.

That is what do-what-I-say means.

Therefore you cannot change what mathematical function they correspond
to.

There are three ways of saying the same only because of a mistake in
the past which became evident only much later. It happens. Get over it.

> > The way to go is to add a new SeparateConvolution filter to Render and make
> > fb call into Pixman to implement it. Once that is in place, the hardware
> > drivers can then implement it.
> >  
> 
> You seem to think the hardware drivers will implement this version of
> SeparateConvolution? Really???

That is implied by do-what-I-say API policy.

> The purpose of this was so that the hardware drivers could implement
> something they call "good" and something they call "best" using whatever
> hardware resources are available. This is not going to happen if they are
> expected to read the filter array.

That will violate the do-what-I-say policy.

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

That won't help this patch get through.

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

Users do not use Pixman. Pixman is an implementation detail of Render
and Cairo. Pixman *is* a low level API, lower than Render or Cairo.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/pixman/attachments/20160307/0e1dfa91/attachment.sig>