[OpenJDK 2D-Dev] Review request for 8029339 Custom MultiResolution image support on HiDPI displays

Jim Graham james.graham at oracle.com
Wed Jul 29 01:42:13 UTC 2015


Hi Alexandr,

All of the new changes look good.  Only one minor issue in a comment in 
the test:

MRRHT.java, line 85: should be "is not based on rendered size" or 
something to that effect
   (No need for a new webrev to fix that print statement.)

I have a separate question out to Phil on whether or not using sun.* 
classes like this in a java.* test is good practice, but the worst case 
answer for that question would be "or should the test be moved to a 
sun.* package"...

One follow-on issue to file and think about - the use of 
getDefaultTransform() requires the construction of a brand new transform 
every time it is used.  For the call to it from the SG2D constructor 
that is fine because that becomes our initial AffineTransform that we 
would have had to create anyway.  But, if they set the DPI_FIT hint then 
we end up calling that method every time we are looking at an MR image. 
  We should look for a way to cache that value, or use internal API that 
doesn't require a clone, or some other way to avoid that "on every 
drawImage call" object creation.  Unless you have an obvious fix you 
want to do now, I don't think we need to hold up this push to get the 
new MR APIs out there...

			...jim

On 7/28/15 8:33 AM, Alexander Scherbatiy wrote:
>
>   Hello Jim,
>
>   Could you review the updated fix:
> http://cr.openjdk.java.net/~alexsch/8029339/webrev.10/
>    - the suggested comments are updated
>    - the MultiResolutionRenderingHintsTest is updated to use custom
> GraphicsConfiguration and SurfaceData
>
> On 7/23/2015 8:25 PM, Jim Graham wrote:
>> Hi Alexandr,
>>
>> On 7/21/15 7:41 AM, Alexander Scherbatiy wrote:
>>>
>>>    Hello Jim,
>>>
>>>   Thank your for the review.
>>>
>>>   Could you review the updated fix there the suggested comments are
>>> updated:
>>>      http://cr.openjdk.java.net/~alexsch/8029339/webrev.09/
>>
>> Minor comments embedded below.
>>
>>> On 7/14/2015 2:36 AM, Jim Graham wrote:
>>>> AbstractMRI - getGraphics() should include "getGraphics() not
>>>> supported on Multi-Resolution Images" as the exception description
>>>> text.
>>
>> AbstractMRI - the getGraphics() description string contains an
>> embedded '\n' and an embedded '\"', are those typos?
>     Updated.
>>
>>>> BaseMRI - class comment - "The implementation will return the first
>>>> ... satisfy the rendering request."  Add another sentence right there
>>>> "The last image in the array will be returned if no suitable image is
>>>> found that is larger than the rendering request."
>>
>> BaseMRI - whoops, my bad.  "that is larger than" should probably be
>> "that is as large as" to match the <= comparison
>      Updated.
>>
>>>> SG2D.getResolutionVariant - using destRegionWH to calculate
>>>> destImageWH can involve a lot of "do some math and then later have
>>>> additional code undo it".  Would it be faster to just compute
>>>> destImageWH directly, as in:
>>>>
>>>> float destImageWidth, destImageHeight;
>>>>
>>>> if (BASE) {
>>>>     destImageWH = srcWH;
>>>> } else if (DPI) {
>>>>     destImageWH = (float) abs(srcWH * devScale);
>>>> } else {
>>>>     double destRegionWidth, destRegionHeight;
>>>>     if (type) {
>>>>     ...
>>>>     }
>>>>     destImageWH = (float) abs(srcWH * destRegionWH / swh);
>>>> }
>>>> Image rv = img.getRV(destImageWH);
>>
>> For the BASE and DPI_FIT cases shouldn't you use srcWidth,srcHeight
>> instead of sw,sh?  The sw,sh are affected by sub-image parameters, but
>> srcWidth,srcHeight are literally the size of the original image, which
>> is what you want to search the MRI for.  The sw,sh should only be used
>> to scale the srcWidth in the "else" clause - as in "srcWidth *
>> destRegionWidth / sw".
>
>     You are right. sw and sh are sizes of a region in the original
> image. I have updated the fix to use srcWidth/Height.
>
>>
>>>> Is there intent to have the default case be mapped to DPI_FIT for
>>>> Windows?
>>>      I think yes.
>>
>> I didn't see where this was being done - is that a TBD follow-on fix
>> or did I miss a default somewhere?
>     I think that it could be fixed with the updating WToolkit to load
> high-resolution toolkit images in the same way as it is done in LWCToolkit.
>     At least it can be filled as a separate issue.
>
>>
>>>> MRRHTest - why not render to a BufferedImage and avoid Robot? Could it
>>>> tap into internal APIs to create a BImg/compatibleImage with a given
>>>> devScale?
>>>
>>>     The DPI_FIT test includes case there a graphics transform is
>>> identity but the device configuration transform has scale 2.
>>>     There should be a way to set scale for the device configuration
>>> transform;
>>
>> Ah yes, DPI_FIT needs a default transform.  Also, without a way to
>> manually create a device with a transform, that means that DPI_FIT is
>> only successfully tested if the tests are run on a HiDPI machine, right?
>>
>>>> MRRHTest - why allow up to delta=50 on the comparison?
>>>      It is just for monitors that are calibrated on Mac OS X and they
>>> colors are different from that which was set. The test uses colors which
>>> have at least one color component differ by 255.
>>
>> Another issue that might go away if we could use BImg instead of robot.
>>
>>>> MRRHTest - since the scale factor comes from a dialog, we depend on
>>>> running this on a HiDPI display to fully test the hints? Could using
>>>> "scaled compatible images" allow testing this more definitively?
>>>
>>>     Could you explain more what does it mean "scaled compatible images"?
>>
>> I seem to recall that there is a mechanism in there to create
>> backbuffer images for swing that record the fact that they are
>> scaled.  I forget how this is done, but I'm wondering if it could be
>> used to run all of this test code in a simulated scale environment
>> rather than using the actual configurations that AWT creates for the
>> current system.
>
>     I have updated the test to use custom GraphicsConfiguration and
> SurfaceData. This allows to use custom scales both on the graphics
> configuration and on graphics.
>
>     Thanks,
>     Alexandr.
>
>>
>>             ...jim
>



More information about the 2d-dev mailing list