[cairo] [Pixman] Gradients patches

[Soeren Sandmann]

Andrea Canciani <ranma42 at gmail.com> writes:

> On Mon, Oct 4, 2010 at 12:07 PM, Andrea Canciani <ranma42 at gmail.com> wrote:
> > This morning I pushed
> > http://cgit.freedesktop.org/~ranma42/pixman/log/?h=radial-for-master
> > It should be ready for master since it is documented, tested (at least
> > on my laptop) and not using
> > any new features (so it should not be broken on other
> > architectures/compilers/etc).
> 
> As suggested on IRC, I tried to cleanup the patch and improve code reuse.
> The result is http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/radial-master
> Except for differentiation, there is basically no clever trick and the code
> should look quite straightforward and minimal.

Indeed, the code looks mostly fine to me; I couldn't find any
functional problems with it. The main complaints I have:

- In the affine case, some of the computation is done in 32.32, and
  some in double precision. Is there any reason for this? Doing all of
  it in double precision would let us get rid of the dot() function
  and the fdot() one could then be renamed to just dot().

- The various values that don't depend on the coordinates (a, cdx, cdy
  etc) can be precomputed and stored in the radial struct. The current
  code does that (and at the very least, if you are not going to
  precompute them, the existing ones should be removed).

- I think both the big comment and the commit message should mention
  the section of the PDF spec where the gradients are specified.

- Formatting. Please format according to CODING_STYLE. In particular:

    - braces on their own line

    - blank lines

        - between logically distinct pieces of code

          For example there should be a blank line between the
          computation of db and the computation of dc and ddc since
          these relate to two different variables (b and c).

        - after blocks of variable declarations

        - in the big comment that describes the equation

      Also, please indent the formulas to set them off from the
      rest of the text and surround them with blank lines.

> I'm omitting the crosspost to the X mailing list because I checked
> again the RENDER extension and it currently only allows radial
> gradients with one circle completely contained in the other one (and
> in this case the old definition gives the same results as the new
> one).

I think this *does* need to be cross posted to xorg-devel because even
though the spec says that it only allows gradients where one circle is
contained in the other, in practice, it doesn't check for that. And
cairo does not check that the circles are contained within eachother
before sending them off to the X server.

> Not all of them were actually true, so I took the linear-float branch and
> worked on it:
> http://cgit.freedesktop.org/~ranma42/pixman/commit/?h=wip/linear-master
> I cleaned it up and made it more robust with respect to error propagation.
> The performance is just slightly worse than its fixed-point counterpart
> (around 10% on x86_64), but it has the big advantage of avoiding all the
> overflows.
> (For the cairo-interested ones, this fixes gradient-linear-large).
> 
> I didn't change linear_gradient_classify (), but it should be corrected or,
> if it does not provide a measurable performance gain, removed.
> (The code seem to indicate that now the only two meaningful classes
> are UNKNOWN and HORIZONTAL).

HORIZONTAL is still important because for horizontal gradients, only
one scanline needs to be rendered; that one can be repeated over and
over. See general_composite_rect(). But yes,
linear_gradient_classify() should be corrected. If vertical is not
used in the gradient code itself anymore, that enum value can be
removed.


Soren