As there has been no update to the supersampling patch posted by Krzysztof Kosiński in 2010 [1] I have rebased the patch and incorporated Søren's review comments [2]. The patches are at:
http://cgit.freedesktop.org/~ajohnson/pixman/log/?h=supersampling My responses to the review comments are below. On 24/08/10 01:09, Soeren Sandmann wrote: > Krzysztof Kosiński <tweenk...@gmail.com> writes: > >> Hello >> >> This is the second attempt at bitmap supersampling in Pixman. > > Some general comments: > > I generally like where this is going. It's certainly a large quality > improvement, and the code is well-written. And of course, it fixes > what is probably the biggest image quality issue in pixman. > > As I mentioned elsewhere, I don't think there should be any > automatically computed supersampling. While it might be nice to make > image scaling look good without any changes to cairo, I think we are > going to want more fine grained control over the API. Also, simply > changing pixman behavior will cause the X server behavior to change, > which seems undesirable. So by default pixman should still just point > sample. > > A question is, what should the API look like? It is probably useful to > make sure we can user other algorithms than regular supersampling in > the future. Suggestions welcome. As a result of an irc discussion with Søren the suggested api is: typedef enum { PIXMAN_SAMPLE_POINT, PIXMAN_SAMPLE_BOX } pixman_sample_t; void pixman_image_set_sampling (pixman_image_t *image, pixman_sample_t sampling); The default sampling is PIXMAN_SAMPLE_POINT. When sampling is PIXMAN_SAMPLE_BOX the supersampling rate is automatically computed. > > Some more specific comments on the code: > > - Is it possible to share the supersampling code between the affine > and projective cases? Maybe the super sampling could be done in an > inline function that takes other inline functions as arguments. I could not figure out a good way to do this. > > - The NO_SUPERSAMPLING flag needs to be added all over the place. None > of the existing fast paths can deal with super sampling. Also, maybe > it should be renamed to POINT_SAMPLING instead; that would get rid > of the negative in the name. I've renamed it to POINT_SAMPLING and added it where ever it appeared to be needed. > > - Can we get away with using a simpler algorithm? I can see why you do > the computation of all the remainders and I appreciate the attention > to correctness. > > However, in most cases, the rate_x/rate_y is going to be small > compared to the fixed point unit of 65536, so it might be possible > to just compute a "small step" and a "big step": > > normal_step = ux/rate_x > initial_step = (ux - (rate - 1) * big_step) / 2 > > and then just use those instead of the remainder computations. I've removed the remainder computations. > >> + uxstart = (ux * (rate_x-1) + rate_x) / (2*rate_x); > > This seems to compute a uxstart of > > ux / 2 - ux / (2 * rate_x) > > which later gets subtracted from the transformed x coordinate to get > the first subsample coordinate. So far, so good. ux/(2 * rate_x) is > the small initial step inside the transformed pixel, and we do need to > subtract ux/2 to get to the corner of the pixel. But then it seems > that _before_ actually sampling for the first time, you end up adding > ux_frac: > >> + spx += uxfrac; > > which offsets the position by uxfrac. I've moved the small step addition to the end of the loop. > > Also, just to make sure, the "+ rate_x" in the first expression is a > rounding correction, and that is the reason it shouldn't converted to > fixed point? (Adding a fixed point and an integer triggers a red flag > for me, but it might be correct). I don't know. I left it as is. > > Finally, I suspect getting rid of the four per-pixel divisions with > samples is going to be a noticable performance win. It should be > possible to just compute (1/samples) up front and then multiply with > it. Fixed. > > > Thanks, > Soren > > -- > cairo mailing list > ca...@cairographics.org > http://lists.cairographics.org/mailman/listinfo/cairo [1] http://lists.cairographics.org/archives/cairo/2010-August/020490.html [2] http://lists.cairographics.org/archives/cairo/2010-August/020655.html _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
