[OpenJDK 2D-Dev] <AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

Anton V. Tarasov anton.tarasov at oracle.com
Fri May 23 11:21:59 UTC 2014


Ok, good!

Anton.

On 23.05.2014 15:18, Anthony Petrov wrote:
> On 5/23/2014 3:12 PM, Anton V. Tarasov wrote:
>> On 23.05.2014 14:47, Anthony Petrov wrote:
>>> 1. The host bounds are not related to the /content/. Hence, adding
>>> this method to the LightweightContent interface would look
>>> inconsistent from API perspective.
>>
>> It's not strictly about content (the name of the interface is not good
>> enough).
>>
>>    32  * The interface by means of which the {@link JLightweightFrame}
>> class
>>    33  * communicates to its client application.
>
> Right. We might want to file a follow-up issue targeted for JDK/FX 9 and rename the 
> interface/rearrange some APIs. However, personally, I don't feel this is strictly necessary at 
> this time.
>
> PS. I'll be pushing the FX part of the fix today. So we should consider the current interface 
> frozen for now.
>
> -- 
> best regards,
> Anthony
>
>>
>>
>>>
>>> 2. Given the requirement to keep backward compatibility, the default
>>> implementation of the method would return 'null', so the calling code
>>> would have to check the return value and fall back to calling
>>> LF.getBounds() manually. Currently this logic is encapsulated in the
>>> LightweightFrame class itself, which looks reasonable to me.
>>>
>>> 3. SwingNode already calls other APIs on LF, such as
>>> notifyDisplayChanged() (and again, this particular notification is
>>> unrelated to the /content/ itself.) So adding the setHostBounds() to
>>> LF looks consistent from this perspective, too.
>>>
>>> 4. The current implementation of the getHostBounds() method simply
>>> returns a new rectangles using cached values. If we implement your
>>> suggestion, then every call to CPLWW.getGraphicsDevice() would involve
>>> an additional call to the SwingNode code, which may impact the
>>> performance slightly.
>>>
>>> 5. I was almost ready to push the FX part of the fix today, and let's
>>> admit it, this fix is very well overdue. I'd prefer if we don't modify
>>> the interface anymore.
>>
>> Ok, this is about a matter of taste. The 5th point sounds strong enough
>> for me, so I'm fine with the current version.
>>
>> Thanks!
>> Anton.
>>
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>>
>>> On 5/23/2014 2:11 PM, Anton V. Tarasov wrote:
>>>> Hi Sergey,
>>>>
>>>> Thanks for the update. I'm fine with the fix, except one thing. (I'm
>>>> sorry that I didn't say that earlier).
>>>>
>>>> My concern is that we have the LightweightContent iface which is used to
>>>> communicate to the client app. And so the method
>>>>
>>>> LightweightFrame.getHostBounds()
>>>>
>>>> is better to be a method of that iface which the client (SwingNode) will
>>>> implement on its side. In that case we won't need the
>>>> LightweightFrame.setHostBounds() method.
>>>>
>>>> This would look consistent from my perspective.
>>>>
>>>> Thanks,
>>>> Anton.
>>>>
>>>> On 22.05.2014 22:05, Sergey Bylokhov wrote:
>>>>> On 5/22/14 5:58 PM, Anton V. Tarasov wrote:
>>>>>> On 22.05.2014 15:36, Sergey Bylokhov wrote:
>>>>>>> On 5/22/14 11:47 AM, Anton V. Tarasov wrote:
>>>>>>>> Hi Sergey,
>>>>>>>>
>>>>>>>> On 22.05.2014 1:44, Sergey Bylokhov wrote:
>>>>>>>>> On 5/21/14 10:13 PM, Anthony Petrov wrote:
>>>>>>>>>> Hi Sergey,
>>>>>>>>>>
>>>>>>>>>> The original fix provides some updates and clarifications to the
>>>>>>>>>> javadoc for the LightweightContent.imageBufferReset() method, but
>>>>>>>>>> they are missing from your fix. Is this intentional?
>>>>>>>>> Nope. I just missed this update. I looked at this method closely
>>>>>>>>> and got a question: do we need this scale parameter? Why we cannot
>>>>>>>>> use w,h + scanstride here an skip all clarification about logical
>>>>>>>>> coordinates?
>>>>>>>>
>>>>>>>> Originally, Jim suggested to generalize the API:
>>>>>>>>
>>>>>>>> <<Rather than imply any parameters, I think specifying a very exact
>>>>>>>> set of parameters gives the most flexibility. Even if the
>>>>>>>> relationships you characterize above are true, xywh,scan or
>>>>>>>> off,wh,scan both provide the flexibility to supply the data in
>>>>>>>> those formats without the client having to guess dimensions or scan
>>>>>>>> size. Any API that specifies an array containing data should always
>>>>>>>> provide the flexibility of specifying an offset (and x,y is a way
>>>>>>>> of specifying an offset for rectangular data and using a nio Buffer
>>>>>>>> can implicitly imply an offset based on its position) and when that
>>>>>>>> data is a rectangle of data then it should also supply independent
>>>>>>>> w,h and scan strides.  If the offset is always 0, and if the
>>>>>>>> scanstride is always w in the implementation's that choose the data
>>>>>>>> storage then it may seem like overkill, but it provides the
>>>>>>>> flexibility of switching to a more sophisticated buffer re-use
>>>>>>>> strategy later without having to track down every client and update
>>>>>>>> them... >>
>>>>>>>>
>>>>>>>> and so we provide all the coordinates.
>>>>>>> I understand why we need x/y/w/h/scanstride but why we need scale,
>>>>>>> because our buffer is pixel based anyway? In this case we have to
>>>>>>> convert w/h/x/y/scanstride from logical to pixels and back.
>>>>>>
>>>>>> The reasoning for that if the following. On the client side
>>>>>> (SwingNode), during the rendering of the image, there's a need to
>>>>>> have logical bounds of the image. So, this would require conversion
>>>>>> (devision)  for what the client would need to know the scale factor
>>>>>> for what it would need to ask for it, separately. This would bring
>>>>>> another code path & dependencies (for instance, b/w SwingNode and its
>>>>>> prism peer). Currently, there's only one parameter of a method for
>>>>>> that purpose.
>>>>> Ok. Here is an updated version:
>>>>> http://cr.openjdk.java.net/~serb/8029455/webrev.02
>>>>>>
>>>>>> Thanks,
>>>>>> Anton.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Anton.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> BTW, I've applied your fix and tested it with the latest version
>>>>>>>>>> of FX changes, and everything works smoothly on my Mac, including
>>>>>>>>>> the display change listener.
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> best regards,
>>>>>>>>>> Anthony
>>>>>>>>>>
>>>>>>>>>> On 5/21/2014 7:46 PM, Sergey Bylokhov wrote:
>>>>>>>>>>> Hello, Everybody.
>>>>>>>>>>> Please review an updated version of this fix. This is a minimum
>>>>>>>>>>> possible
>>>>>>>>>>> fix. changes in shared code of jdk are minimized and can be
>>>>>>>>>>> enhanced in
>>>>>>>>>>> the future.
>>>>>>>>>>> The fix is covering hdpi support in SwingNode on osx + system
>>>>>>>>>>> look and
>>>>>>>>>>> feel(Aqua).
>>>>>>>>>>>
>>>>>>>>>>> http://cr.openjdk.java.net/~serb/8029455/webrev.01
>>>>>>>>>>>
>>>>>>>>>>> Notes:
>>>>>>>>>>>   - This fix depends from two other fixes: JDK- 8041129 and
>>>>>>>>>>> JDK-8041644.
>>>>>>>>>>> Both are under review on 2d alias.
>>>>>>>>>>>
>>>>>>>>>>> On 5/13/14 9:29 PM, Anthony Petrov wrote:
>>>>>>>>>>>> Hi Jim, Sergey, and Anton,
>>>>>>>>>>>>
>>>>>>>>>>>> I'd like to revive this old thread and finally push this fix,
>>>>>>>>>>>> which
>>>>>>>>>>>> has been reviewed and approved on this mailing list back in
>>>>>>>>>>>> February.
>>>>>>>>>>>> The only additional change that I want to introduce, is the
>>>>>>>>>>>> addition
>>>>>>>>>>>> of default implementations for the
>>>>>>>>>>>> LightweightContent.imageBufferReset() methods. This allows
>>>>>>>>>>>> clients of
>>>>>>>>>>>> the API (namely, JavaFX) to run with both the old and the new
>>>>>>>>>>>> JDK w/o
>>>>>>>>>>>> any changes or side-effects. Here's the updated webrev:
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~anthony/9-2-hiDPISwingNode-8029455.0/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> It literally only adds the default methods and doesn't make any
>>>>>>>>>>>> other
>>>>>>>>>>>> changes to the rest of the already reviewed code. I've tested
>>>>>>>>>>>> this on
>>>>>>>>>>>> both hiDPI and loDPI displays, with both old and hiDPI-aware
>>>>>>>>>>>> JavaFX
>>>>>>>>>>>> builds, and it works fine in all the cases.
>>>>>>>>>>>>
>>>>>>>>>>>> The current plan is to push this fix to JDK 9, and then
>>>>>>>>>>>> back-port the
>>>>>>>>>>>> changes to 8u20.
>>>>>>>>>>>>
>>>>>>>>>>>> -- 
>>>>>>>>>>>> best regards,
>>>>>>>>>>>> Anthony
>>>>>>>>>>>>
>>>>>>>>>>>> On 2/21/2014 2:47 AM, Jim Graham wrote:
>>>>>>>>>>>>> Yes, approved.
>>>>>>>>>>>>>
>>>>>>>>>>>>>          ...jim
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2/17/14 6:09 AM, Anton V. Tarasov wrote:
>>>>>>>>>>>>>> Jim, so this is ready for a push then.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>> Anton.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 15.02.2014 5:01, Jim Graham wrote:
>>>>>>>>>>>>>>> I don't need to see an update for that.  I didn't read the
>>>>>>>>>>>>>>> entire
>>>>>>>>>>>>>>> webrev, but I looked at this one piece of code and if that
>>>>>>>>>>>>>>> was the
>>>>>>>>>>>>>>> only thing changed then I think that dealt with the
>>>>>>>>>>>>>>> outstanding
>>>>>>>>>>>>>>> issues...
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>         ...jim
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2/13/14 11:12 PM, Anton V. Tarasov wrote:
>>>>>>>>>>>>>>>> On 14.02.2014 2:52, Jim Graham wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2/13/14 5:03 AM, Anton V. Tarasov wrote:
>>>>>>>>>>>>>>>>>> Hi Jim,
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Please, look at the update:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.5
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Here I'm correcting the rect after the transform in SG2D:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 2123         // In case of negative scale transform,
>>>>>>>>>>>>>>>>>> reflect the
>>>>>>>>>>>>>>>>>> rect
>>>>>>>>>>>>>>>>>> coords.
>>>>>>>>>>>>>>>>>> 2124         if (w < 0) {
>>>>>>>>>>>>>>>>>> 2125             w *= -1;
>>>>>>>>>>>>>>>>>> 2126             x -= w;
>>>>>>>>>>>>>>>>>> 2127         }
>>>>>>>>>>>>>>>>>> 2128         if (h < 0) {
>>>>>>>>>>>>>>>>>> 2129             h *= -1;
>>>>>>>>>>>>>>>>>> 2130             y -= h;
>>>>>>>>>>>>>>>>>> 2131         }
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> The blit direction (dx, dy) remains transformed. Is this
>>>>>>>>>>>>>>>>>> the right
>>>>>>>>>>>>>>>>>> behavior from your perspective?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes, that looks good.  I wonder if the "w *= -1" results
>>>>>>>>>>>>>>>>> in a
>>>>>>>>>>>>>>>>> multiply
>>>>>>>>>>>>>>>>> byte code whereas "w = -w" would avoid the multiply?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>             ...jim
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Jim,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yes, this indeed results in different byte code
>>>>>>>>>>>>>>>> instructions (imult &
>>>>>>>>>>>>>>>> ineg). Just for curiosity I did some measuring which showed
>>>>>>>>>>>>>>>> negatioation
>>>>>>>>>>>>>>>> worked 10% faster :)
>>>>>>>>>>>>>>>> Well, I'll fix it but let me please not send an update...
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>>>> Anton.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>




More information about the 2d-dev mailing list