[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-10-28 21:47:13
Message-ID: 8c4463d9-82f4-7379-134c-bb20328866fa () oracle ! com
[Download RAW message or body]

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]