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

Wed Aug 19 14:01:42 UTC 2015

Looks fine.

On 19.08.15 14:51, Alexander Scherbatiy wrote:
>
>   Hello,
>
>  Could you review the updated fix:
>     http://cr.openjdk.java.net/~alexsch/8029339/webrev.12
>
>   Javadoc description is updated in some places and parameters 
> checking is added:
>   - @throws IllegalArgumentException tag is added for 
> MultiResolutionImage.getResolutionVariant(destImageWidth,destImageHeight) 
> method
>   - Comment about nonempty return list is added for 
> MultiResolutionImage.getResolutionVariants() method
>   - BaseMultiResolutionImage class description is updated to mention 
> getResolutionVariant(destImageWidth, destImageHeight) method
>   - @throws IllegalArgumentException and NullPointerException tags are 
> added for BaseMultiResolutionImage  constructors
>  - parameters checking is added for BaseMultiResolutionImage, 
> MultiResolutionCachedImage and MultiResolutionToolkitImage clases
>  - Exception message is updated for MRRHT.java, line 85
>
> Thanks,
> Alexandr.
>
>
> On 7/29/2015 4:42 AM, Jim Graham wrote:
>> 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
>>>
>

-- 
Best regards, Sergey.