Andrea Canciani ranm...@gmail.com writes:
On Mon, Oct 4, 2010 at 12:07 PM, Andrea Canciani ranm...@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
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
