[OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some fractional scales

[Jim Graham]

I suppose that there could be a 4th bug about the fact that the trimming/refining code in the native TransformHelper is 
based on an inverse transform and subject to bit error, but it's not easy to figure out how to refine it further.  Note 
that it could also cause us to omit a row of pixels in some cases if the rounding went the other way.  Refining it 
further could be easier than the 2nd two solutions, but not really noticeable if we just get do the more conservative 
calculations on the bounds equations in the setup code...

			...jim

On 10/28/16 2:33 PM, Jim Graham wrote:
> Hi Alexandr,
>
> That code is coming up with maximum extents for the operation, not the exact edges that will be rendered.  It passes
> them down to the native TransformHelper method which refines them using the exact calculations it will use to render the
> pixels.
>
> Admittedly, the calculations you point out below in the DrawImage pipeline would more accurately note that the
> right/bottom edges fall on a .5 pixel location and shouldn't be included, but I didn't want to make that decision using
> one set of equations while the actual rendering uses different math down below.
>
> Note that if you had a rotation then there are lots of unused pixels on each scanline which are computed and refined by
> the native code.  Those calculations would also be performing the "included/excluded" calculations using the actual
> rendering equations rather than these higher level boundary equations so even if we switched to "ceil(N - 0.5)"
> calculations up here for the bounding box to cure the scaled drawImage we'd still potentially have overshoot due to the
> fixed point/floating point bit errors on those rotated edges.
>
> With a general transform it would be difficult to detect the occasional extra pixel being rendered due to bit errors,
> but it is much easier with these scale-only transforms.
>
> One change we could make would be to do the ceil(N-0.5) calculation in the bounds setup code which would "fix" the scale
> only case.
>
> Another might be to fix the scale pipeline to handle arbitrary formats - we could write a related "ScaleHelper" native
> method that would do the same thing that TransformHelper does.  TransformHelper takes a transform loop that transforms
> pixels to ARGB, and a separate Blit or MaskBlit that blends ARGB pixels into the destination and it runs the first loop
> into a temporary local buffer on the C call stack and the second loop to blend the pixels into the dest.  A ScaleHelper
> loop would do the same, but with a "Scale(XXX to ARGB)" loop for the first half.
>
> Finally, we could also add more ScaleBlit loops for more source/dest pairs so these scales would happen more directly.
> Unfortunately, many of the missing cases would require creating scale loops that also do blending and the only macros we
> currently have for direct src->dest scaling are opaque-to-opaque pixel store operations.
>
> The first solution is simpler and easier to add, the second one would help with all scales that fall outside our loops.
> The last solution would arguably provide the highest performance for any source/dest case that we care about (and with
> the advent of HiDPI we now care about a lot more cases).
>
> I'd file 3 bugs:
>
> - Scaled and transformed drawImage can overshoot with some scales (1.5)
> - Add more ScaledBlit graphics primitive instances for common HiDPI cases
> - Create a general purpose Scale helper method along the lines of TransformHelper
>
> The first could be fixed soon with the proposed change you talk about below...
>
>             ...jim
>
> On 10/28/16 6:40 AM, Alexandr Scherbatiy wrote:
>> On 10/11/2016 10:13 PM, Sergey Bylokhov wrote:
>>> On 11.10.16 21:54, Jim Graham wrote:
>>>> Additionally, for AA rendering, there is no such thing as
>>>> "setClip(someShape)" and "fill(sameShape)" being identical no matter how
>>>> much we predict which rules they may want because clips are inherently
>>>> non-AA and so the fuzzy pixels along the boundary of an AA shape will be
>>>> randomly clipped or included.
>>>>
>>>> When all is said and done I feel (not very strongly) it would be best to
>>>> have clipping remain unaffected by the biasing of STROKE hints, but part
>>>> of that is driven by the fact that I think it can be fixed with a couple
>>>> of lines of code in SG2D/LoopPipe...
>>>
>>> I guess a bottom line is that it is require an additional investigation. I am not exactly sure, but my expectation is
>>> that fillRect(x,y,w,h)/drawImage(x,y,w,h) + setClip(x,y,w,h) should work in a similar way(covers the same pixels). And
>>> the question should this behavior be exactly the same as fill(RectShape) + setClip(RectShape) is unclear (I am not
>>> sure is it a critical issue or not)
>>> Right now I tried to fix overlapping, which produce visible artifacts and were considered as a bugs. The next issue
>>> which I would like to fix is a overlapping of drawImage().
>>   I just created a small sample [1] which fills a rectangle (x, y, w, y), creates an image with size (w, h) and draws
>> the image into the area (x, y, w, h).
>>   With the floating point scale like 1.5 the image does not exactly fits the area filled by the rectangle (see image [2]
>> generated by the code [1]).
>>
>>  This is because the Graphics.drawImage(...) calls DrawImage.renderImageXform() which uses floor and ceil methods for
>> the region corner rounding:
>>         final int dx1 = Math.max((int) Math.floor(ddx1), clip.getLoX());
>>         final int dy1 = Math.max((int) Math.floor(ddy1), clip.getLoY());
>>         final int dx2 = Math.min((int) Math.ceil(ddx2), clip.getHiX());
>>         final int dy2 = Math.min((int) Math.ceil(ddy2), clip.getHiY());
>>
>> But the Graphics.fillRect() falls down to the code which just cast the transformed coordinates to int.
>>
>> Why the floor/ceil methods are used for the image region rounding?
>> Is it possible to change this to fit the rule for the filled rectangles rounding?
>>
>> [1] http://cr.openjdk.java.net/~alexsch/fpapi/code/FillRectAndImageTest.java
>> [2] http://cr.openjdk.java.net/~alexsch/fpapi/screenshots/rect-and-image.png
>>
>> Thanks,
>> Alexandr.
>>>
>>>
>>