On 11/30/2016 2:11 AM, Jim Graham wrote:
I agree that the two versions of the paintDoubleBuffered functions look like they may not have any noticeable performance difference. It would be nice to avoid the duplication if they don't, I had assumed that such a performance test had already been done...?
    There could be 2 problems
- There were checks that Graphics g is instanceof Grpahics2D. If it fails it is necessary to fall down to the scale 1. - The BufferedImage is used if the volatile image buffer is disabled. It seems it is necessary to keep both pixels coordinates and sizes for both graphics the passed and the buffer.


I would prefer to keep the current fix as is and investigate the problems with the non volatile offscreen image and the paintDoubleBufferedImpl method elimination in a separate issue: JDK-8170593 Non volatile OffscreenBuffer should be scaled in RepaintManager for HiDPI display
      https://bugs.openjdk.java.net/browse/JDK-8170593

  Thanks,
  Alexandr.

clipRound() has appropriate logic for matching the results of filling a path. clipScale() is a more basic "just scale this value and clip it" type of operation that has uses - not everything needs to match the rasterization logic of fills. I did a quick grep to see where clipScale() is used:

- SG2D.constrain(, ... region) - this method needs to be overhauled as it currently relies on scaling a region which is not a valid operation - only paths can be scaled, once they are rasterized into a region it is too late to adjust their coordinate transform, but I think fixing this involves fixing shaped components to remember their outline shape rather than an outline region. :(

- SG2D.drawHiDPI() - this fix changes one of the instances of using clipScale() in that method, but not the other one. They should probably both match. I don't have a good answer for which is the best rounding method to use here in any case since the original concept was that we'd extract the integer pixels and pretend that they were an isolated image, but if we end up with a fractional scale then that isn't really possible. It may be better to just leave this alone for now. Does anything about the rest of this fix depend on that change? Hopefully if RepaintManager does its job right we end up painting the back buffer to the destination all on integer pixel boundaries on both the source and dest and so rounding isn't an issue?

- Region.getScaledRegion() - this method should be deleted. I think it is only used in SG2D.constrain() which needs to be rewritten as you cannot scale a region and expect any amount of accuracy, but we live with it for now until a better fix comes along. I don't think changing the rounding method used by this method will have any useful impact, it just needs to be eliminated by fixing the callers to no longer need it.

There are a couple of more uses in the AWT that I didn't look too closely into at this point, but those are the uses within the Java2D area...

            ...jim

On 11/24/16 8:40 AM, Sergey Bylokhov wrote:
I have a few questions which probably discussed already, then ignore it:

- SunGraphics2D.java: As far as I understand the clipScale() was replaced by clipRound(), because they have different round logic? It seems that when I wrote the clipScale() I was not aware about round logic, and looks like we can change the clipScale implementation to use clipRound internally instead of Math.round(newv), can be fixed by othe fix. - Did you check the difference in performance between paintDoubleBufferedImpl vs paintDoubleBufferedFPScales? At least in terms of heavyweight operations it looks similar, and probably we can have only one of them? It has an additional
benefits that the new code will be tested on the usual system as well.

On 21.11.16 16:59, Alexandr Scherbatiy wrote:

Hello,

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

- isFloatingPointScale(AffineTransform) is moved from the SunGraphics2D
to the SwingUtilities2 class.

  Thanks,
  Alexandr.

On 11/18/2016 11:23 PM, Jim Graham wrote:
Hi ALexandr,

This looks great.

BTW, when I suggested moving the FPscale test into SG2D I was
suggesting that to avoid having to copy the transform out of it via
getTransform(), but you've found a different solution to that issue
(i.e. the new getTransform(g) method) so it no longer matters where
that utility static function is located.  You can move it back to one
of the Swing classes.

In terms of the logic of choosing which repaint function to use, it
looks like you use the old-style function if the scales don't match,
but won't that cause rendering anomalies?  The new code is still an
improvement for the standard HiDPI case, and I'm guessing that
mismatched scales probably never tends to happen, but we might want to
flag it for further investigation.

+1 relative to whether you want to move the FPscale test back out of
SG2D or not...

            ...jim

On 11/18/16 1:44 AM, Alexandr Scherbatiy wrote:

Thank you. I see that using the integer device-pixel translations
preserves the component painting in the same way for
floating point scales.

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

  - translation adjustment is removed
  - Region.clipRound() is used for pixels coordinates rounding.

  Thanks,
  Alexandr.

On 11/16/2016 1:52 AM, Jim Graham wrote:
Let me clarify something...

On 11/15/16 2:49 AM, Alexandr Scherbatiy wrote:
  Let's consider the following use case:
  scale = 1.5
  A component calls fillRect(1, 1, 1, 1).
  This is (1.5, 1.5, 3.0, 3.0) in the device space
  which fills  (1, 1, 3, 3) and covers 2x2 pixels

Agreed.

  Now the area (1, 1, 1, 1) needs to be repainted
    create a backbuffer
    translate(-1, -1) // move the top left corner of the area to
the zero point
    draw the component into the backbuffer:
      fillRect(1, 1, 1, 1) -> after translation fillRect(0, 0, 1,
1) -> after scaling  (0.0, 0.0, 1.5, 1.5 ) in the
device space
      which fills (0, 0, 1, 1) and covers 1x1 pixels

If you did g.setTransform(identity), g.translate(-1, -1), (then
restore the scale) then the analysis is as follows:

g.setTransform(identity) => [1 0 0] [0 1 0]
g.translate(-1, -1) => [1 0 -1] [0 1 -1]
g.scale(1.5, 1.5) => [1.5 0 -1] [0 1.5 -1]
g.fillRect(1, 1, 1, 1)
    => coordinates are (1.5-1, 1.5-1, 3-1, 3-1)
    => (.5, .5, 2, 2)
    => fills (0, 0, 2, 2)
    => which covers 2x2 pixels

If you did g.translate(-1, -1) on the scaled transform then the
analysis is as follows:

g.transform is [1.5 0 0] [0 1.5 0]
g.translate(-1, -1) is [1.5 0 -1.5] [0 1.5 -1.5]
g.fillRect(1, 1, 1, 1)
    => coordinates are (1.5-1.5, 1.5-1.5, 3-1.5, 3-1.5)
    => (0, 0, 1.5, 1.5)
    => fill (0, 0, 1, 1)
    => covers 1x1 pixels

The second operation is what you are describing above and that would
be an inappropriate way to perform damage repair
because you used a scaled translation which did not result in an
integer coordinate translation.

Please re-read my previous analysis that shows what happens when you
use integer device-pixel translations which are
translations that happen using integers on a non-scaled transform.
Note that you can add a scale *AFTER* you apply
the integer device pixel translation and it will not affect the
integer-ness of the translation.  You can see above
that the difference in how the translate command is issues affects
where the translation components of the matrix end
up being -1,-1 or -1.5,-1.5...

            ...jim





Reply via email to