[Pixman] [PATCH/RFC 1/2] New FAST_PATH_SIMPLE_ROTATE_TRANSFORM flag

[Siarhei Siamashka]

On Friday 29 October 2010 03:35:10 Soeren Sandmann wrote:
> Siarhei Siamashka <siarhei.siamashka at gmail.com> writes:
> > And additional somewhat related observation. Simple translate transforms
> > are currently taking nearest scaling fast paths, which is definitely a
> > lot better than general path. But we surely can do better. Combination
> > of flags
> > 
> >     FAST_PATH_SCALE_1X |
> >     FAST_PATH_X_UNIT_POSITIVE  |
> >     FAST_PATH_Y_UNIT_ZERO |
> >     FAST_PATH_MATRIX10_ZERO |
> >     FAST_PATH_AFFINE_TRANSFORM |
> >     FAST_PATH_NEAREST_FILTER |
> >     FAST_PATH_SAMPLES_COVER_CLIP
> > 
> > could be used to select a simple nonscaled and nonrotated optimized
> > translate transform fast paths. Or alternatively translate transforms
> > could be handled in a special way by just updating 'src_x'/'src_y'
> > variables. But these flags still may be useful for fractional
> > translations with bilinear filter .
> 
> It could be interesting to go the other way too: If all the fast paths
> got support for pure translation matrices, then we might be able to
> drop the src/mask_x/y variables from the composite functions and just
> absorb them into the matrix.

Yes, but that's an API break at least. Also with unpredictable outcome for
performance.

> > What I'm trying to say is that optimizations for transformed compositing
> > operations in pixman seem to be "converging" and starting to cover more
> > use cases. And the flags are becoming quite descriptive.
> 
> It may be at some point that the whole flag scheme stops working so
> well. For example, it could be that the flags eventually become so
> descriptive that the fast path cache would be filled with small
> variations on the same set of flags.
> 
> In practice, that doesn't seem to happen yet, but it's worth keeping
> an eye on.

If this happens, the problem has to be solved by introducing a better cache. I
did not pay much attention to this earlier, but looks like the cache is
checking for exact match of the cached entries now? It indeed makes sense for
caching fast path lookup misses and also allows not to worry about the order of
matched functions. But in this case using a hash looks like a perfect choice.

> > Anyway, unless there are some comments/objections, I'm going to commit
> > this patch with initial rotation flags support a few days after pixman
> > 0.20.0 release.
> 
> The patch looks fine to me, though generally I'd prefer to have the
> flags pushed at the same time as the code that uses them, just to
> avoid a situation where the first part of something is pushed, but the
> rest of never happens.

The flags are also useful by themselves even if the optimizations are not yet
hooked to them, at least for the "slow path" reporting code (admittedly it also
needs to be added to pixman first).

Regarding the rotation, the difficulty here is that it's hard (or even 
impossible) to find some real world application which uses this functionality
via cairo/xrender/pixman. In this case cairo-trace could be created and the
need for introducing this optimization would be fully justified. But I guess 
people avoiding the use of pixman rotation is more like the case of:
    Patient: Doctor, it hurts when I do this!
    Doctor: Well, then don't do that!

Another problem is that there are competing solutions which are (at least
perceived to be) a much better choice for doing this work. For example GPU or
a dedicated hardware in a display controller. Nevertheless it seems like
optimized rotation still may be needed at least as a backup solution in some
cases which are important for us, so I'm going to ensure that the needed
optimizations get added to pixman. The branch with the latest rotation fast
path code is here:
http://cgit.freedesktop.org/~siamashka/pixman/log/?h=fast-simple-rotate-20101026

Additionally I already got a private e-mail in the last few days asking when
pixman is going to get ARM NEON optimized rotation. But I actually would prefer
the people to express their interest on the mailing list, at least for better
visibility of the demand for such a feature.

> > +		else
> > +		if (image->common.transform->matrix[0][1] ==  pixman_fixed_1 &&
> 
>                 ^
>                 These should be on the same line.

Thanks, I will fix that.

-- 
Best regards,
Siarhei Siamashka