[OpenJDK 2D-Dev] <AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Anthony Petrov
anthony.petrov at oracle.com
Fri May 23 15:22:03 UTC 2014
On 5/23/2014 6:51 PM, Kevin Rushforth wrote:
>> PS. I'll be pushing the FX part of the fix today. So we should
>> consider the current interface frozen for now.
>
> Yes, please!
Here it is:
http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/9f221ab57e22
--
best regards,
Anthony
>
> -- Kevin
>
>
> 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