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

[Sergey Bylokhov]

But probably it is possible to tweak the transform? I mean that we can 
apply the "round", then calculate the diff between resulted destination 
and destination based on transform, and shift/rescale the transform so 
it will map exactly the pixels from source edges to destination edges.

On 29.10.16 0:47, Jim Graham wrote:
> 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.
>>>>
>>>>
>>>


-- 
Best regards, Sergey.