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

[Alexander Scherbatiy]

   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
>>