[Pixman] [PATCH] Don't discriminate PAD and REFLECT repeat in standard fast paths

[Soeren Sandmann]

Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:

> On Tuesday 28 September 2010 17:24:14 Siarhei Siamashka wrote:
> > The flags in pixman are implemented in such a way that they all have some
> > kind of "positive" meaning, indicating that the operation is going to be
> > more simple than the general case in one way or another. Having more flags
> > set on the initial analysis stage is always good. Forgetting to set some
> > of the flags is safe in the sense that it will not make pixman misbehave,
> > pixman will just run slower because of not taking some fast paths.

To rephrase: the flags should be though of as properties that the
images have.

   If the FAST_PATH_BLAH flag is set, then the image has property
   BLAH.

   If a fast path includes BLAH in its mask, then it is saying "I can
   only run if the image has property BLAH".

For fast paths, that's really all there is to it, bugs in the
PIXMAN_STD_FAST_PATH macro notwithstanding.

So the logic we rely on is:

  BLAH is set  =>  image has property BLAH

But there is no need for the arrow to go the other way because if an
image has property BLAH and the flag isn't set, then we just won't run
that particular fast path.

Ie., the flags have a positive meaning, as you say.

> Tried to actually verify this claim by artificially clearing flags in 
> 'compute_image_info' and running pixman tests. Appears that not everything
> is so simple.
> 
> Some flags are a bit special:
> - NARROW_FORMAT flag needs to be propagated to 'general_composite_rect'
> function because it is used there
> - calculation of NEEDS_WORKAROUND flag also can't be skipped

That is a good point. 

Yes, in the cases you point out (NARROW_FORMAT and NEEDS_WORKAROUND),
we do make use of the negative meaning of the flag. If NARROW_FORMAT
is *not* set, then wide fetchers are used. So, we rely on this:

  BLAH is not set => image does not have property BLAH

which by contraposition gives

  image has property BLAH  =>  BLAH is set

So those two flags really are special in that they *must* be set if
the image actually has the property.

This is actually kind of bad. I don't like special cases, and I hadn't
realized we had one here. But maybe we can get rid of NEEDS_WORKAROUND
in 0.22, and if we add a floating point implementation as a fallback
for the general one, that would allow the general one to just put
NARROW_FORMAT flags on its "fast path", using the flag in a more
conventional way.

> - flags ID_TRANSFORM, NO_ALPHA_MAP, NO_CONVOLUTION_FILTER, NO_PAD_REPEAT
> and NO_REFLECT_REPEAT are needed to select untransformed fetchers 
> bit_image_fetch_untransformed_32/bit_image_fetch_untransformed_64 instead of
> bit_image_fetch_general/_pixman_image_get_scanline_generic_64, and apparently 
> these behave differently

This is in *principle* the same positive use of the flags. It should
in *principle* be possible to ignore all these fetchers and just use
the bits_image_fetch_general() function. However, there are
differences because when you have a transformation, the 64 bit
pipeline is actually using 32 bit pixels expanded to 64 bits.

Which is broken, of course. So hopefully, this is another case where a
floating point implementation can help. However, for that to work, we
will need a couple of things from the floating point implemenetation:

- The floating point implementation must do transformations and
  gradients (unlike the existing 64 bit pipe).

- We need the ability to plug in implementation specific
  fetchers. Otherwise we can't do proper fallback from the general to
  the floating point implementation.

So if Dmitri and Jonathan are still reading, I think the above points
are worth keeping in mind.

Here are some notes on how CPU specific fetchers could be done:

http://cgit.freedesktop.org/~sandmann/pixman/tree/docs/fetchers?h=docs&id=2dcb46955869ed042d1bc32f738ab23ab569b32e


Soren