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

[Laurent Bourgès]

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:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.1/


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

Fixed

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
>

Yes.

          - 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 <<
SUBPIXEL_LG_POSITIONS_Y) = ceil(spmaxY)


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

I reported all changes in RendererNoAA.

Laurent


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