Gradients for Xrender

[Lars Knoll]

On Thursday 02 June 2005 17:59, David Reveman wrote:
> On Thu, 2005-06-02 at 10:07 +0200, Lars Knoll wrote:
> > On Wednesday 01 June 2005 19:02, David Reveman wrote:
> > > On Mon, 2005-05-30 at 19:22 +0200, Lars Knoll wrote:
> > > Nice to see that someone is finally doing some work on gradients for
> > > Render.
> > >
> > > I've had a quick look at the xserver patch and I've got a few questions
> > > about the patch and some comments about the protocol changes. I'll
> > > start with the protocol changes.
> > >
> > > I'm not a big fan of sticking the gradients into a read-only Picture. I
> > > don't like that we'll have a bunch of picture attributes that only
> > > works with certain types of pictures. Can you set a source clip for a
> > > gradient? alpha map? filter? component alpha?
> >
> > A read only picture is rather easy to do, and I don't think we gain
> > anything by separating the types. IMO it would make the whole API harder
> > to use and implementation in the server more difficult). An additional
> > type doesn't give you a whole lot more then some syntactic sugar.
> >
> > > Having repeat and pad[*]
> > > attributes in pictures and also a spread type in gradients is very
> > > confusing. I think the extend attributes for pictures need to be fixed
> > > and it seems like a good idea to use them for gradients. I think the
> > > extend types supported by glitz are good: Transparent, Nearest, Repeat
> > > and Reflect. I'm not sure how useful Transparent is with gradients but
> > > it's easy to implement, so for consistency we might just support that
> > > with gradients too. I've got a patch for libpixman that implements
> > > these extend types. If people think it's a good idea, I'll make sure it
> > > gets into libpixman and the xserver soon.
> >
> > That would mean either
> > (a) changing the repeat member of the XRenderPictureAttributes from a
> > boolean to an int. Since we're in C we can do this without breaking
> > compatibilty (provided Transparent = 0 and Repeat = 1).
> > or (b) adding a second spread attribute and deprecate repeat.
> >
> > Both ways I wouldn't mind moving the attribute in there, and adding
> > Transparent as an additional spread for gradients.
> >
> > Does anyone have any strong opinions about the two alternatives (a) or
> > (b)?
>
> so (a) would mean:
>
> repeat { TRANSPARENT = 0, REPEAT = 1, REFLECT = 2, PAD = 3 }
>
> It would break apps that use something else than 1 for TRUE but I don't
> know if that's a major concern.

That was basically my question. I don't mind adding another attribute neither, 
we just need to decide on which way to do.

I'm not sure if e.g. this could break on some platforms: 
attr.repeat = (foo == bar);

If it doesn't and repeat would be 1 after this operation, we could probably be 
reasonably safe with changing the type. Otherwise we should go for a new 
attribute.

> > > We might also want to use the filter attribute with gradients but I
> > > don't think anything but the filter aliases (Fast, Good, Best) makes
> > > sense for gradients and I'm not sure what to do about that.
> >
> > Well, none of them makes sense, as we can calculate the gradient point
> > (0...1) exactly even with transformations (and without additional
> > overhead). So you'll always get "Best". But we shouldn't complain if
> > anyone tries to set them.
>
> I was thinking about per pixel sampling here, not the accuracy of the
> gradient calculation. You'll get aliased edges between color stops when
> you're only computing one sample per pixel. For most gradients you wont
> see this but when you have color stops that are close together or use
> spread Transparent it will look bad.

Yes. Spread will have this problem, so it might be a good idea to define the 
filters for how the sampling is done. 

> > > I like the pattern idea that Owen mentioned earlier better than
> > > read-only pictures, but that's a pretty large change to Render and I
> > > understand if people like to avoid that. If we fix the extend
> > > attributes, I think I can live with the read-only pictures for
> > > gradients.
> > >
> > > In your patch, I see that you're computing a color table when creating
> > > a gradient. If you render gradients that are larger than the color
> > > table or if you just use a transformation that enlarge the gradient a
> > > lot, the results can be very wrong.
> >
> > As long as you have not too many stop points you should be ok. The fixed
> > point precision does limit you to a maximum of 65000 stops anyway.  The
> > color table could be wrong if someone adds a 1000 stops, but that's more
> > of a theoretical problem.
>
> This would only be true if you have a separate color table for each
> color stop span. Doesn't look like this is what you're doing. The size
> of each color table would have to be as large as the number bits per
> color component of the destination format if we don't want to loose
> precision. A color table solution that would do something like this,
> doesn't seem like a good idea to me.
>
> Test case (all numbers in 16.16 fixed point):
>
> linear_gradient.from.x = 0;
> linear_gradient.from.y = 0;
> linear_gradient.to.x = 256 << 16;
> linear_gradient.to.y = 0;
>
> stop[0].color = RED;
> stop[0].offset = 0;
> stop[1].color = GREEN;
> stop[1].offset = 1;
> stop[2].color = BLUE;
> stop[2].offset = 1 << 16;
>
> transform_fixed = {
>   1,       0,       0,
>   0, 1 << 16,       0,
>   0,       0, 1 << 16
> };
>
> Composite this gradient over a 256x256 area and you should get a smooth
> gradient from RED to GREEN. Does this work with your code?

Sure it doesn't. As I said to be exact _and_ fast we probably need both the 
exact stops and a color table and decide in the composite call which to use.

> > We've tried doing the calculations directly in our Qt code, and found out
> > that you can gain a factor of 5 in speed using a precalculated color
> > table. We could store both the exact stops and a precalculated color
> > table in the server if this really turns out to be a problem.
>
> Yes, using color tables will give you a nice performance improvement. I
> just want to make sure that you know that there are a lot of cases where
> the precalculated color table isn't good enough. Both color tables and
> exact stops are probably what we want to have.
>
> > > Are the color stops pre-multiplied or non-pre-multiplied? We'll need to
> > > support non-pre-multiplied color stops. Maybe we want to support
> > > pre-multiplied color stops as well, I'm not sure.
> >
> > Why would you need non premultiplied color stops? You can always convert
> > them to premultiplied on the client side. All of the Render API currently
> > expects premultiplied colors and I see no reason to change that.
>
> When premultiplying color stops you'll loose information needed to
> compute correct color stop interpolation. Imagine the case when you want
> to do a have gradient from solid white to fully translucent white:
>
> { 1.0, 1.0, 1.0, 1.0 } ->  { 1.0, 1.0, 1.0, 0.0 }
>
> this cannot be done with premultiplied color stops as we'll send these
> color stops to the server:
>
> { 1.0, 1.0, 1.0, 1.0 } ->  { 0.0, 0.0, 0.0, 0.0 }

Which will still work correctly, as the interpolated values will contain the 
correct colors. 

But I do see your point, as full white to a translucent red will not be 
possible to parametrize without adding an additional stop in the middle.

On the other hand Render did up to now only use premultiplied alphas, and for 
everything else I would not want to remove this restriction. I would not like 
to add non premultiplied Picture formats.

> > > I'd like to see radial gradients with support for an inner radius, at
> > > least in libpixman, as we could then throw away my software gradient
> > > code in cairo and use libpixman instead. It's actually a lot harder to
> > > implement the inner radius than just a focal point. My inner radius
> > > code in cairo seems to be working fine but it's not very efficient and
> > > it's all in floating point. Let me know if you figure out a smarter way
> > > of doing them.
> >
> > It's not hard to do an inner radius. If you need it I can add it to our
> > gradient code.
>
> That would be great!

I can easily do it as long as we can have the restricion that one circle lies 
completely inside the other one. If they overlap or lie outside of each other 
the behaviour is a lot harder to define (postscript does it, but the 
calculations get rather tricky in this case).

Lars


> > > Overall I think the patch looks good. I'll hook up glitz's gradient
> > > acceleration in Xgl once this goes into CVS.
> >
> > Good :)
> > I'll submit it then as soon as we have an agreement on the final protocal
> > and client API.
>
> Yes, do so. Implementation issues like if we can use color tables or not
> don't have to stop us from getting this code into CVS.