Gradients for Xrender

[David Reveman]

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.

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

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

> 
> 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 }
 
> 
> > 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!

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


-David