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

Anthony Petrov anthony.petrov at oracle.com
Thu May 22 20:45:09 UTC 2014


Hi Sergey,

> http://cr.openjdk.java.net/~serb/8029455/webrev.02

The fix looks fine to me.

--
best regards,
Anthony

On 5/22/2014 10:05 PM, 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