[cairo] [Pixman] Gradients patches

[Andrea Canciani]

On Mon, Oct 11, 2010 at 12:56 AM, Soeren Sandmann <sandmann at daimi.au.dk> wrote:
> 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().

To keep the whole precision, using 32.32 is needed. If we want to use
double there, we should at least stop doing the differences trick (and still
the precision would be lower).

>
> - 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.

It's section 8.7.4.5.4 (Type 3 (Radial) Shadings) of the PDF reference manual.

>
> - 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.

I see that now xorg-devel is in the CC addresses, anyway Cairo should pay
more attention to what it does. Latest RENDER specification says:
The array of stops has to contain values between 0 and 1 (inclusive) and
has to be ordered in increasing size or a Value error is generated. The inner
circle has to be completely contained inside the outer one or a Value error is
generated.

>
>> 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.

I'll prepare another patch to fix this.

The attachment is a new patch for the radial gradient implementation,
where I tried to apply the correction you asked for, except for the type
of b and c (and db, dc, ddc) which stayed the same.

I changed the variable naming a little to make the "delta" variables
a circle_t (so that its type is consistent with that of c1 and c2).
I will look into changing the formulas to make this more obvious (maybe
replacing the "d" of cdx, cdy and dr with a Δ (delta) would be enough).

Andrea
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Draw-radial-gradients-with-PDF-semantics.patch
Type: application/octet-stream
Size: 18933 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20101011/0e8f73e3/attachment-0001.obj>