[OpenJDK 2D-Dev] <AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Thu May 22 18:05:38 UTC 2014
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.
>>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>>
>
--
Best regards, Sergey.
More information about the 2d-dev
mailing list