[OpenJDK 2D-Dev] Various fixes to pisces stroke widening code

[Jim Graham]

Hi Denis,

More thoughts on Renderer.java.

-- Skipping gaps (minor optimization) --

If there is a gap in the edges in Y, say if a path consists of two 
subpaths, one that covers y=[0..10] and another that covers 
y=[1000..1010], then I think you will iterate over each y value from 10 
to 1000, but have no work to do on each scan line.  That could possibly 
waste a bit of time.

On the other hand, fixing that would have to take into account whether 
or not you are done with a given alpha row, so the "NextY" function 
can't simply skip - the skipping has to be done at the higher level in 
endRendering - or at least with the cooperation of endRendering.  Since 
it asks what the "current Y" is near the top of the loop, it could 
detect if the current Y jumped out of the given alpha row, emit it, and 
prepare for a new alpha row starting at the new Y...?

For now, it might be simpler to ignore this and revisit that later since 
it isn't going to be common to have large such jumps in the middle of 
most paths.

-- Done with skipping gaps --

-- Alpha accumulation opt --

Filling the alpha row.  I had an interesting optimization here that I 
never got around to.  Instead of filling the array entries with the 
alpha values, fill them with the deltas of the alpha values.  The inner 
loop in endRendering then becomes something like:

     alpha[pix_x0  ] += NUM_POS - (x0 & MASK);
     alpha[pix_x0+1] += (x0 & MASK);
     alpha[pix_x1  ] -= NUM_POS - (x1 & MASK);
     alpha[pix_x1+1] -= (x1 & MASK);

The [pix+1] lines were the gotcha that always confused me when I tried 
to do this in the past.  Basically if you enter an inside region at 1 
subpixel before the end of a pixel then you need to add one to the 
coverage for that pixel.  But, starting with the next pixel you want the 
total contribution of the interior region to be NUM_POS per pixel, but 
you've only added 1 so far - so you have to add the additional amount so 
that the total amount added for each "enter" crossing ends up being 
NUM_POS.  Similarly, when you decrement for the "exit" crossing you need 
to ensure that the total negative delta sums to NUM_POS across the pixel 
where the exit happens and the following pixel.  Does that make sense?

Note that you could still have the "single pixel" optimization which 
would look like:

     alpha[pix_x0  ] += (x1 - x0);
     alpha[pix_x0+1] -= (x1 - x0);

and is equivalent to the above 4 lines.

then when you do the RLE, you simply have to use an accumulation as you 
scan the alpha row:

     byte nextVal = startVal + alphaRow[i];

So, for the cost of an add per pixel as you do the RLE you can avoid 
having to do the loop in the "multiple pixel section" when filling the 
alpha array.

I don't think I've ever quantified this optimization so it might be 
better to investigate it after the current code goes in and adopt it 
only if it shows a big savings.

-- Done with alpha accumulation opt --

Something about the "emit last row" code bothers me.  First, I would 
guess that the "y == boundsMaxY - 1" test would have already flushed the 
row, right?  Also, why do the for loop?  Why not just flush the row if 
it isn't flushed?  I kind of feel like the "y == boundsMaxY-1" test 
could be removed from inside the loop and simply test if there is 
unflushed data in the alpha array then just make a call to emitRow() 
with the appropriate values after the loop.

     while (hasNext()) {
         ...
         if ((y & MASK) == MASK) {
             emitRow(...);
             pix_min,max = MAX,MIN;
         }
     }
     if (pix_min < pix_max) {
         emitRow(...);
     }

Am I missing something?

So, the upshot is that I didn't find anything wrong.  You can take or 
leave my suggestions for improvements as you see fit and maybe save some 
of them for a second round after this goes back...

			...jim

On 7/29/2010 2:16 PM, Denis Lila wrote:
> Hello Jim.
>
> I made the changes you point out, except for your second point
> (I don't have time to think about it right now).
>
> I stopped precomputing m00_2_m01_2 because it was only being
> used in one place. But I guess it worth it to precompute it
> if it's going to be used often (and computeOffset is called
> for every line).
>
> The new webrev is at http://icedtea.classpath.org/~dlila/webrevs/fpBetterAAv2/webrev/
>
> Thanks,
> Denis.