Review request: JDK-8170030 Code in Marlin-based rasterizers may have an off-by-1 bug
bourges.laurent at gmail.com
Fri Dec 9 21:51:39 UTC 2016
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
> line 1456 - I can understand why we lose the +1, but not the min() with
Once again, it is no more needed as:
spmaxY <= (pmaxY << SUBPIXEL_LG_POSITIONS_Y)
ie spmaxY <= ((spmaxY + SUBPIXEL_MASK_Y) >> SUBPIXEL_LG_POSITIONS_Y <<
SUBPIXEL_LG_POSITIONS_Y) = ceil(spmaxY)
> 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)
More information about the openjfx-dev