[OpenJDK 2D-Dev] <AWT Dev> [8] Request for review: 8000629 [macosx] Blurry rendering with Java 7 on Retina display

[Sergey Bylokhov]

Hi, Jim.
On 4/9/13 1:19 AM, Jim Graham wrote:
> Hi Sergey,
>
> Did you check for possible performance hits for the new transform 
> save/compare code in BufferedContext?
I am not sure how it can be improved, because BufferedPaints.setPaint() 
depends from the current transform of the sg2d.transform. And it should 
be called, when sg2d.transform is changed.
> That is significantly more work per validate(). 
I guess in most common case only aftm.equals() was added.
> Also, isn't this case only used in the case where the caller specified 
> a null xform in the argument list?  Does that happen (or, I guess, how 
> does it happen) on a scaled graphics?
Yes this happens if xform is null, so only code under if (xform == 
null)  was changed.
>
> Something I didn't notice before - you adjust the max texture sizes in 
> CGLGraphicsConfig by the device scale, but you adjust them by 
> "getScaleFactor() * 2".  Why the "* 2"?
This is related to 7160609. The problem was in fact, that we create huge 
texture during window initialization. In the fix I change this logic to 
use GL_MAX_TEXTURE_SIZE/2.
Separate CR:8010999 was created to track it.
>
> You removed a call to flushOnScreenGraphics in LWWindowPeer.  Was that 
> just being paranoid before and really isn't needed?
Now it is called not only when the window is resized, but when the 
surface is replaced. It was moved to replaceSurfaceData.
>
> It's already done and the code is written, but in retrospect, I'm not 
> sure how much the optimizations in getTransform/setTransform are worth 
> compared to the easy readability of having the transforms done 
> linearly in a LIFO manner, such as:
>
> setTransform:
>     transform.setToIdentity();
>     if (cX|cY != 0) { transform.translate(cX, cY); }
>     if (dS != 1) transform.scale(dS, dS); }
>     transform.concatenate(Tx);
> getTransform:
>     tx = new AT(transform);
>     if (dS != 1) { tx.scale(invDs, invDs); }
>     if (cX|cY != 0) { tx.translate(-cX, -cY); }
>     return tx;
>
> (I don't think it should be changed, but I'm throwing it out there as 
> food for thought as it may be easier to maintain in the future, but it 
> could also change the performance as I'm just speculating from how I 
> remember the optimizations work...)
When it was written, I was impressed by a code in the 2D.
>
>             ...jim
>
> On 4/8/2013 6:10 AM, Sergey Bylokhov wrote:
>> Hi, Phil, JIm.
>> New version of the fix:
>> http://cr.openjdk.java.net/~serb/8000629/webrev.08
>>   - Assumption that we should scale all native surfaces was wrong. In 
>> case of managed Bufferedimage we shouldn't scale native surface, 
>> because they are copies 1-1.(CGLSurfaceData.java:60).
>>   - BufferedContext is not always updates current Paint, because it 
>> checks transX/transY from sg2d, which is not maintained in the scale 
>> mode.
>> Test: FillTexturePaint.java
>>
>> Also I found a problem in the sg2d.getTransform(), where invScale 
>> does not apply to constrainX/Y.
>> Test: TransformSetGet.java
>>
>> On 4/5/13 10:00 PM, Phil Race wrote:
>>> SFAIK you haven't fixed the problem with TexturePaint I pointed out 
>>> last week.
>>> That needs to be fixed before you push.
>>>
>>> -phil.
>>>
>>> On 4/5/2013 9:54 AM, Sergey Bylokhov wrote:
>>>> Hi, Jim.
>>>> I assume, that you haven't additional comments? Can I consider you 
>>>> as the second reviewer?
>>>> Note that I plan push it to awt-dev because, my other fixes depend 
>>>> on it.
>>>>
>>>> On 3/26/13 7:33 PM, Sergey Bylokhov wrote:
>>>>> Hello,
>>>>> Please review the fix for jdk 8.
>>>>> Change adds initial support of hidpi(mostly on 2d side). In the 
>>>>> fix scale was added to the surface data/CGraphicsDevice /CGLLayer. 
>>>>> This scale factor maps virtual coordinates to physical pixels.
>>>>> This change doesn't add support of hidpi to aqua l&f and doesn't 
>>>>> add support of dynamic change of scale factor.
>>>>>
>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000629
>>>>> Webrev can be found at: 
>>>>> http://cr.openjdk.java.net/~serb/8000629/webrev.06
>>>>> -- 
>>>>> Best regards, Sergey.
>>>>
>>>>
>>>> -- 
>>>> Best regards, Sergey.
>>>
>>
>>
>> -- 
>> Best regards, Sergey.
>>


-- 
Best regards, Sergey.