[Pixman] [PATCH 2/3] Optimize BILINEAR filter to NEAREST for identity transforms

[Soeren Sandmann]

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

>> But this breaks down if the compositing code starts *relying* on the
>> flags being set, for example to know what the filter is; it will no
>> longer be the case that "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".
>>
>> This is an important rule to preserve, because it allows people to work
>> on the flag computing code without knowing the details of the
>> compositing code, and it allows people to write a new compositing
>> routine or implementation without knowing that the flags even exist.
>
> I see it more like as an extra convenience feature. It's just because
> some flags may be difficult to set precisely, so that it is always set
> when we do have some feature and not set when we don't. One example is
> SAMPLE_OPAQUE flag (we would need to check all the pixels of a8r8g8b8
> images when setting flags to be sure that we never miss any opaque
> image, and this naturally does not make sense). But some of the flags
> may be easily set precisely.

The rule has at least two benefits: First, the code that computes flags
is allowed to just not set some flags if it would be inconvenient. We
take advantage of this in this piece of code:

    /* Both alpha maps and convolution filters can introduce
     * non-opaqueness in otherwise opaque images. Also
     * an image with component alpha turned on is only opaque
     * if all channels are opaque, so we simply turn it off
     * unconditionally for those images.
     */
    if (image->common.alpha_map                                 ||
        image->common.filter == PIXMAN_FILTER_CONVOLUTION       ||
        image->common.component_alpha)
    {
        flags &= ~(FAST_PATH_IS_OPAQUE | FAST_PATH_SAMPLES_OPAQUE);
    }

where some unlikely special cases means the opaque flags can't be
set. If the opaque flags had to be precise, this issue couldn't be dealt
with in such a simple way, and writing and reviewing the code would
become harder.

Second, currently, if compositing code is relying on flags for negative
information, it is almost certainly broken. With this patch, compositing
code would be in some cases *required* to treat the flags in a negative
way, and in other cases *broken* if it did. This means such code can no
longer be written or reviewed without referencing the documentation for
individual flags.

>> When such general rules break down is when the code base becomes
>> incomprehensible and only understandable to people who are intimately
>> familiar with it.
>
> The rules just need to be properly documented. Fast path flags can
> have a longer comment section, explaining what they actually mean and
> whether they are precise.

Having to document individual flags in greater detail is another symptom
of a misdesign going on.

> Even though both FILTER_NEAREST and FILTER_BILINEAR flags may be valid
> for certain operations, setting both of them may be no very practical,
> or even harmful. Just one example. Currently for ARM target we
> implement bilinear scaling using NEON instructions (fast path
> registered in 'pixman-arm-neon.c'), but nearest scaling does not need
> it, so we do it with just ARMv6 (with fast path registered in
> 'pixman-arm-simd.c'). Now let's suppose that we have both
> FILTER_NEAREST and FILTER_BILINEAR flags set for some scaling
> operation. And in this case NEON bilinear fast path will be selected
> instead of a better nearest fast path which is available further in
> the list after fallback.

This example is an instance of a more general problem:

   http://www.mail-archive.com/pixman@lists.freedesktop.org/msg00888.html

It's not an easy one to fix. Setting at most one of the filter flags is
a band-aid for one particular case, not a fix for the underlying
problem. It may be worth applying this band-aid and avoiding setting
both flags, but this could be done in the COVER_NEAREST/BILINEAR flags
as well.

The main point I wanted to make is that it would be *legal* (and
potentially useful) to set both. It wouldn't make pixman behave
incorrectly.

> I still think that optimizing the filter as early as possible in the
>pipeline is the best choice. So that the rest of the pipeline just does
>not need to care. And while it's probably just more like a band aid
>(the performance is still not going to be great), optimized filter is
>also somewhat helping here:
>http://cgit.freedesktop.org/pixman/tree/pixman/pixman-bits-image.c?id=pixman-0.22.0#n565

If this patch were the *only* way to fix an important performance
problem, it could be considered, but in this case there is a
straightforward other way: just compute COVER_BILINEAR/COVER_NEAREST
flags. That doesn't break the 'flags are interpreted positively' rule,
and doesn't impose additional rules on the compositing code such as "you
must access the filter through the flag bits, not through the regular
variable".


Soren