[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [9] Review Request: 8167310 The graphics clip is incorrectly rounded for some f
From:       Jim Graham <james.graham () oracle ! com>
Date:       2016-11-02 23:10:38
Message-ID: 0183330f-84a5-3735-584f-5c7a0b3b1427 () oracle ! com
[Download RAW message or body]

Tweaking might reduce the number of cases where the reverse transform 
makes a different edge condition decision than the forward transform, 
but it will never completely eliminate it and so the refine code (which 
exists because forward computations don't always match reverse 
computations) would still be needed.  Both could be part of the same fix 
- make the refinement more accurate and also consider if a better 
approximation of the transform would make the refinement actions more 
rare...

			...jim

On 11/1/2016 8:30 AM, Sergey Bylokhov wrote:
> 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.
>>>>>
>>>>>
>>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]