[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:12:02 UTC 2014
On 23.05.2014 14:47, Anthony Petrov wrote:
> Hi Anton,
>
> I disagree, and here's my arguments:
>
> 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.
>
> 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