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

[Alexander Scherbatiy]

  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