[OpenJDK 2D-Dev] <AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Anthony Petrov
anthony.petrov at oracle.com
Fri May 23 11:18:03 UTC 2014
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