<Swing Dev> [9] Review request for 8162350 RepaintManager shifts repainted region when the floating point UI scale is used

Tue Nov 29 17:46:11 UTC 2016

On 11/24/2016 7:40 PM, 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.
   The clipRound() is used to be consisted with the fillRect(...) rounding.
>
>  - 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.

   The result of running SwingMark 2 with the following JDK is:
   1. without the fix
     1st test run [1]: 92373
     2nd test run [2]: 92156

    average: (92373 + 92156) / 2 = 92264.5

   2. paintDoubleBufferedImp() method is always used
     1st test run [3]: 92476    // (92476 - 92264.5) / 92264.5 / 100 = 
0.000023%
     2nd test run [4]: 90800  // (90800 - 92264.5) / 92264.5 / 100 = 
-0.000159%

   3.paintDoubleBufferedFPScales () method is always used
     1st test run [5]: 91053    // (91053 - 92264.5) / 92264.5 / 100 = 
-0.000131%
     2nd test run [6]: 92900    // (92900 - 92264.5) / 92264.5 / 100 = 
0.000069%

Thanks,
Alexandr.

[1] 
http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-base_00.txt
[2] 
http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-base_01.txt
[3] 
http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-int_00.txt
[4]http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-int_01.txt
[5] 
http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-fp_00.txt
[6] 
http://cr.openjdk.java.net/~alexsch/8162350/swingmark2/repaint-manager-fp-scale-fp_01.txt
>
> 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
>>>>
>>
>
>