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

[Jim Graham]

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?

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

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

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

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

			...jim