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

[Jim Graham]

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