[9] Review request: JDK-8170030 Code in Marlin-based rasterizers may have an off-by-1 bug

Laurent Bourgès bourges.laurent at gmail.com
Fri Dec 9 21:51:39 UTC 2016

Dear Jim,

Normally we'd isolate fixes for different bugs even if they are just
> preparatory formatting changes on comments.

I agree but we are short on time and I consider those syntax changes are
minor and without risk.

Here is an updated webrev:

> Some comments on the formatting changes:
> line 35 - you should also upper-case the E


line 148 - I don't understand why this "float" was a formatting danger, but
> the one on line 328 isn't?  Not a big issue, but curious since I think the
> edges array does contain floats in it, doesn't it?

It was an old comment that is no more true: the off-heap edges only contain
integer values via _unsafe.putInt(addr,...) in addLine()

> line 328 - (ints) => [ints] to match?  (Also would match line 148)

Fixed for consistency.

> Comments on the bug fix:
> line 1401 - why does maxY want to be bounds+1?
>           - later this is used to calculate buckets_maxY


          - do we want an extra Y bucket for some reason?

No more, apparently. It was certainly needed before due to the improper
handling of half-open intervals.

          - and why only in case of spmaxY was clipped?
>           - a comment about that would help
> Other than that one odd case of using "+1" there, that whole block of code
> is just:
>     spmaxY = min(edgeMaxY, _boundsMaxY)
> which is much easier to read.
> Perhaps we need an extra bucket only when there were clipped edges below
> the clip?

I fixed all that conditions and tested again with Marlin (MapBench
regression tests) and it is now very obvious and simpler, thanks for the
tip !

> line 1456 - I can understand why we lose the +1, but not the min() with
> pmaxY?
Once again, it is no more needed as:
     spmaxY <= (pmaxY << SUBPIXEL_LG_POSITIONS_Y)
ie  spmaxY  <= ((spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y <<

> NoAA comments are identical, though line numbers may be different...

I reported all changes in RendererNoAA.


> On 12/9/16 8:46 AM, Laurent Bourgès wrote:
>> Please review this simple fix for MarlinFX:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8170030
>> webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.0/
>> PS: I will provide asap a Marlin2D patch incorporating changes made in
>> MarlinFX (including this one)
>> Cheers,
>> Laurent

More information about the openjfx-dev mailing list