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