Re: [9] Review request for 8162350 RepaintManager shifts repainted region when the floating point UI scale is used

[Alexandr Scherbatiy]

Hello,
Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8162350/webrev.02

  The previous fix renders the dirty region to the  backbuffer from the 
same (x, y) coordinates where it should be repainted.
  The JScrollPane can have large (x, y) values which can exceed the 
double buffer maximum size and some regions can be painted outside the 
backbuffer.

  The current fix tries to adjust the component translation to a value 
which allows to draw a component in the same way when floating point 
scale is used.
  The scale is converted to the irreducible fraction n / m where m is 
the step under which the component is drawn in the same way.
  The translation to the zero point is adjusted to the value: 
-translation + translation % m.
  The backbuffer is enlarged to the value: size + m.

  The floating point scale and general transform are handled 
differently because general transform requires calculation pixel values 
in general way, not just scale * x + translation.
  The integer and floating point scales are handed differently because 
the floating point scale handling now use % operator which can consume 
some time.

  Thanks,
  Alexandr.

On 11/1/2016 11:23 PM, Jim Graham wrote:
Is SunGraphics2D accessible from Swing?  If so, then I'd recommend 
putting the isFPScale() method right on that class so we don't have to 
clone the transform by calling g.getTransform().  Also note that 
g.getTransform() does more than clone the transform as it has to 
factor out the constrainXY translation - which are always integers so 
it won't have any effect on the results of "isFPScale()" and is 
additional wasted work.  Eventually we could tie this into the 
transformState variable in SG2D to differentiate integer and fp scale, 
but that would take a little more work as those flags are used in a 
lot of places - for now we can at least get rid of the transform clone 
and the constraint translation processing.

In the implementation of isFPScale(tx), do you really want to return 
false for non-scale transforms?  It seems to me that if the transform 
has rotations or shears in it then we might need to punt and just 
repaint the whole viewport.  Also, you could simplify it a little to 
avoid an extra "getter" and extra bit math:

isFPScale(AffineTransform tx) {
    int type = tx.getType() & ~(FLIP | TRANSLATE);
    if (type == 0) {
        return false;
    } else if (type == SCALE) {
        // check for integers
    } else {
        return true or false?;
    }
}

The changes to SG2D.drawHiDPIImage point out that we should probably 
allow fp subimage paramters in the image pipeline for better accuracy, 
but that's a much bigger change.  Until then sub-image blits are not 
going to be accurate on scaled images. Won't this inaccuracy affect 
our back buffer blits in Swing?

The changes to RepaintManager took me a couple of tries to figure out. 
It looks like you are now rendering the dirty region at the 
appropriate X,Y location in the back buffer (rather than at 0,0) in 
all cases to adjust for the fact that rendering the same primitive at 
different locations doesn't always match.  First, you expand the back 
buffer even for the unscaled case which wasn't affected by HiDPI.  
Second, as long as the translate is in device pixels, it shouldn't 
matter where in the buffer you render it, so it should be enough to 
just ensure integer translations - did you try using an integer origin 
for the rendering instead?

            ...jim

On 10/24/2016 9:11 AM, Alexandr Scherbatiy wrote:

  Hello,

  Could you review the updated fix:
    http://cr.openjdk.java.net/~alexsch/8162350/webrev.01
  - JScrollPane copy area functionality is disabled for floating 
point scale
  - VolatileImage buffer size is recalculates using transform translate

  Using floating point scale leads that drawing the same thing from
different coordinates gives different results. For example filling a
rectangle with size (1, 1) from location (0, 0) and UI scale 1.5 gives
scaled region (0, 0, 1.5, 1.5) which is rounded to (0, 0, 2, 2). The
same rectangle filled from the location (1, 1) gives the scaled region
(1.5, 1.5, 3, 3) which is rounded to (2, 2, 3, 3). The first rectangle
has size 2 in the device space and the second one has 1.
  As a result drawing a component from some coordinates and using
Graphics.copyArea() to translate an image to a new location could have a
differ results than just drawing the same component from the new 
location.
  The fix suggests to disable the JScrollPane area copying for floating
point scales.

Thanks,
Alexandr.

On 10/7/2016 4:30 PM, Alexandr Scherbatiy wrote:
On 10/6/2016 11:42 PM, Sergey Bylokhov wrote:
Hi, Alexandr.
Can you please provide some standalone small example, which emulates
this artifacts via java2d API. (The pattern which we use in
RepainManager). It will help to understand the problem.

The code sample [1]  draws two the same shapes (with different colors)
one after another into areas (x, y, w, h) and (x+w, y, w, h)
accordingly in different ways.

The shape is constructed from the following parts:
1. Fill clip area
 - set clip (x, y, w, h)
 - fill the whole image
As a result only clipped area is filled.

2. Fill rect
- fill rect(x, y, w, h) // big rect
- fill rect(x+1, y+1, w-2, h-2) // small rect

3. Draw center lines
- draw line (x, cy, x + w, cy)
- draw line (cx, y, cx, y + h)

The program has the following options:
  RECT - draw two shapes one after another from point (0, 0)
  SHIFTED_RECT - draw two shapes one after another from point (x, y)
  BACKBUFFER - draw the shape into a backbuffer with size (w, h) and
draw the backbuffer from the point (x, y)
  SCALED_BACKBUFFER - draw the shape into a scaled backbuffer with
size (ceil(w*scale), ceil(h*scale)) with scaled graphics from point
(0, 0) and draw it into the rectangle (x, y, w, h)
  ENLARGED_SCALED_BACKBUFFER - draw the shape into a scaled backbuffer
with size (ceil((x+w)*scale), ceil((y+h)*scale)) with scaled graphics
from point (x, y) and draw it into the rectangle (0, 0, x+w, y+h)

The resulted images are placed in the directory [2].
Directory name "rect-[7,5,10,8]" means that the rectangles (7,5,10,8)
was used for the shape drawing.
Each screenshot name follows the template "
screenshot-N-[x,y,w,h]-TYPE.png" where the type is a program option
used for the image generation.
Screenshots with suffix "-compare" compares the golden image (shape
drawn in to the rectangle (x, y, w, h)) with the generated image. The
golden image is on the top left side. The generated image is shown on
the right and bottom side.

The RepaintManager has an assumption that drawing something in some
area (x, y, w, h) or just drawing the same thing into an image with
translated graphics g.translate(-x1, -y1) and drawing the image into
the area(x, y, w, h)  has the the same result.

As it is shown on screenshots this statement is not true for floating
point scales.
For example the same shape drawn from the point (0,0) and (x,y) look
differently (see [3] and [4]).

The solution could be just to use an enlarged backbuffer with size
(x+w, y+h) with scaled graphics, draw the component into the rectangle
(x, y, w, h) and draw the backbuffer into the area (0,0, x+w, y+h).
Even the scaled enlarged backbuffer is used the results can be differ
(see [5] where the rectangle [7, 5, 11, 9] is used. The backbuffer is
drawn into bigger size).


[1] code samples:
http://cr.openjdk.java.net/~alexsch/8162350/code/00/Java2DFPSamples.java 

[2] dir with screenshots results:
http://cr.openjdk.java.net/~alexsch/8162350/results
[3] RECT:
http://cr.openjdk.java.net/~alexsch/8162350/results/rect-%5b7%2c5%2c10%2c8%5d/screenshot-01-%5b7%2c5%2c10%2c8%5d-rects.png 

[4] SHIFTED_RECT:
http://cr.openjdk.java.net/~alexsch/8162350/results/rect-%5b7%2c5%2c10%2c8%5d/screenshot-02-%5b7%2c5%2c10%2c8%5d-shifted-rects.png 

[5] ENLARGED_SCALED_BACKBUFFER compare:
http://cr.openjdk.java.net/~alexsch/8162350/results/rect-%5b7%2c5%2c11%2c9%5d/screenshot-05-%5b7%2c5%2c11%2c9%5d-enlarged-scaled-backbuffers-compare.png 


Thanks,
Alexandr.


On 06.10.16 20:07, Alexandr Scherbatiy wrote:
Hello,

Could you review the fix:
  bug: https://bugs.openjdk.java.net/browse/JDK-8162350
  webrev: http://cr.openjdk.java.net/~alexsch/8162350/webrev.00

  The fix uses the solution suggest by Jim in the email:
http://mail.openjdk.java.net/pipermail/2d-dev/2016-October/007737.html 


  To draw to a VolatileImage backbuffer its graphics transform is
set to
identity and device coordinates are used to set the buffer clip.
  Copying the backbuffer image to the graphics has some problems.
  -------------
    // Since there is no drawImage(img, float x, float y)...
    destination.translate(pixelx1 / scaleX, pixely1 / scaleY)
    destination.drawImage(img, 0, 0)
  -------------
  This code solves the problem for the top left corner of the region.
All Graphics.drawImage(...) methods scales the image size and it 
looks
like ceil(img.getWidth() * scaleX) can be differ from the
ceil(pixelx1 +
img.getWidth() * scaleX) - pixelx1 so the right bottom corner of the
image does not fit the required point.
  There is also a question could a line drawn from one point and then
from another has a different width in pixels because the graphics 
scale
is not integer.

  The proposed fix prepares a backbuffer with size [x + w, y + h] 
in a
user space and a component is drawn in to the region [pixelx1, 
pixely1,
pixely2, pixely2] in the device space.
  After that the necessary clip is set to the graphics and whole 
image
is just drawn into it.

  The new logic is used only the component graphics configuration is
scaled the graphics configuration has the same scales. So it possible
just to copy the backbuffer surface data to the graphics surface 
data.
  For other cases like for rotated graphics transform it seems it is
necessary to have more complicated algorithm.

  This solves problems with repainted region but there are still
artifacts with JInternalFrame moving or a component scrolling, 
This can
be related to the RepaintManager.copyArea() method which needs to be
updated in the similar way. I have created an issue on it:
   JDK-8167305 RepaintManager.copyArea() method should be updated for
floating point UI scale
     https://bugs.openjdk.java.net/browse/JDK-8167305

 Thanks,
 Alexandr.