<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays

[Jim Graham]

Just to be specific so we don't get bogged down in misunderstandings. 
Here is the modification that I'm suggesting:

3086         } else if (img instanceof MultiResolutionImage) {
3087             // get scaled destination image size
3088
3089             int width = img.getWidth(null);
3090             int height = img.getHeight(null);
3091
3092             Image resolutionVariant = getResolutionVariant(
3093                     (MultiResolutionImage) img, width, height,
3094                     dx1, dy1, dx2, dy2, sx1, sy1, sx2, sy2);
3095
3096             if (resolutionVariant != img && resolutionVariant != 
null) {
3097                 // recalculate source region for the resolution variant
3098
                      ImageObserver rvobserver = 
MultiResolutionToolkitImage.
                                  getResolutionVariantObserver(img, 
observer,
                                          width, height, -1, -1);

3099                 int rvWidth = resolutionVariant.getWidth(rvobserver);
3100                 int rvHeight = resolutionVariant.getHeight(rvobserver);
3101
3102                 if (0 < width && 0 < height && 0 < rvWidth && 0 < 
rvHeight) {
3103
3104                     float widthScale = ((float) rvWidth) / width;
3105                     float heightScale = ((float) rvHeight) / height;
3106
3107                     sx1 = Region.clipScale(sx1, widthScale);
3108                     sy1 = Region.clipScale(sy1, heightScale);
3109                     sx2 = Region.clipScale(sx2, widthScale);
3110                     sy2 = Region.clipScale(sy2, heightScale);
3111
                          observer = rvobserver;
3115                     img = resolutionVariant;
3116                 }
3117             }
3118         }

(And perhaps this explains why I was pushing for the rv dimensions to be 
determined on the fly - or hardcoded at 2x for now - in the wrapped 
observer?)

			...jim

On 12/3/13 9:49 AM, Jim Graham wrote:
> Hi Alexander,
>
> There is one last thing that I think I forgot to mention in SG2D that
> might make some other comments I made make more sense.  There is no
> observer registered on the resolution variant in SG2D.drawHiDPIImage()
> in the case where the resolution variant hasn't been loaded yet.
> Basically, the lines at 3099,3100 will trigger the variant to load, but
> there is no observer in those calls to trace it back to the guy who
> needs to call drawImage() again.  So, the only thing I think that needs
> to be done is that the observer needs to be wrapped and handed in to
> those calls to getWidth/Height(observer).
>
> The rest of that method looks fine - the regular variant will be used
> (and will trigger repaints via the code that calls into drawImage())
> until the base image dimensions are known enough to trigger the
> getResolutionVariant() code, and then we might continue to use the
> regular version until the resolution variant at least knows its
> dimensions, and that is all OK, but we need to start using the observer
> wrapper on the resolution variant starting at lines 3099,3100 in order
> to get the repaints to keep happening for that version of the image.
>
> Arguably, in addition, the unwrapped observer probably could be used on
> lines 3089, 3090 when you get the dimensions of the base image, but
> since the base image will later be handed to the drawImage pipeline, the
> observer will be registered there anyway, so that isn't a bug.  But, the
> wrapped observer needs to be used on 3099,3100 or we may never repaint
> with the resolution variant (it will be a race condition based on how
> fast the regular and hiDPI images load).
>
> More comments below, but that is the only remaining blocker that I can
> see...
>
> On 12/3/13 3:48 AM, Alexander Scherbatiy wrote:
>> On 12/3/2013 1:16 AM, Jim Graham wrote:
>>> On 12/2/13 4:55 AM, Alexander Scherbatiy wrote:
>>>> On 11/30/2013 3:27 AM, Jim Graham wrote:
>>>>> Hi Alexander,
>>>>>
>>>>> I suppose the wrapping solution works for ImageObservers, but I think
>>>>> the suggestion I gave in recent emails was to simply modify the
>>>>> newInfo method (and a couple of other methods) to deliver the same
>>>>> information with no caches, no hashmaps/lookups, no wrapping the
>>>>> observer, and no wrapping with soft references.  Keep in mind that
>>>>> observers are typically references to Component objects so any delay
>>>>> in processing the soft references could keep relatively large
>>>>> component hierarchies in play (if they are parents).  It should work
>>>>> well for a first draft, though.
>>>>      It seems that just updating the newInfo method is not enough.
>>>
>>> There were 5 or 6 places that called imageUpdate when I did a quick
>>> search and most of the calls went through newInfo.  They'd all have to
>>> be updated.  Other than that, I'm not sure why it would not be enough?
>>
>>    Consider the following scenario. There are image.png and image at 2x.png
>> files on the disk.
>>      Image image1 = Toolkit.getImage("image.png");  // load as
>> multi-resolution image
>>      Image image2 = Toolkit.getImage("image at 2x.png");  // load the image
>> from cache
>>      toolkit.prepareImage(image2,.., imageObserver2);
>>
>>     The image2 has image1 as the base image so it rescale its
>> coordinates/dimension and passes the base instead of self to the
>> imageObserver2 which does not look correct.
>
> I see your point now.  I had thought that they would be cached
> separately, but I see now that both are inserted into the hash directly.
>   That allows sharing if they access both manually, but it complicates
> the observer issue.  I don't think I would have bothered with sharing
> the Image instance with a manual reference to the @2x in that case, but
> we should be able to handle both in a future bug fix and hopefully also
> get rid of wrappers, but it would take some surgery on the drawImage
> pipeline and the record keeping in the observer lists.  The existing
> solution will work fine for @2x images and allow sharing so it is good
> to go for now (modulo the one issue with using the wrapper for the
> getWidth()/getHeight() I mentioned above).
>
>>>>> Also, why does ObserverCache exist? Couldn't the cache just be a
>>>>> static field on MRToolkitImage?
>>>>       MRToolkitImage can be used in drawImage(Image,..,ImageObserver)
>>>> method always with null observer. So the is no need to create the
>>>> observer cache or use a synchronization during the cache
>>>> initialization.
>>>
>>> Maybe I'm missing something about your answer here, but I think you
>>> may have misunderstood my question.  You placed the field that holds
>>> the reference to the cache inside an inner class.  I didn't see why
>>> the reference could not be stored in the base class.  Why is there an
>>> empty inner class to wrap a single field?  In other words, why was
>>> this used:
>>>
>>>   56
>>>   57     private static class ObserverCache {
>>>   58
>>>   59         static final SoftCache INSTANCE = new SoftCache();
>>>   60     }
>>>   61
>>>
>>> Instead of just:
>>>
>>>   56
>>>   59     static final SoftCache INSTANCE = new SoftCache();
>>>   61
>>
>>     Just to not create the cache in case if MRToolkitImageis used but
>> image observers are always null. See the comment above.
>
> Ah, so this is just a micro-optimization then?  I'm not sure what you
> mean by "always null".  It depends on whether they hand in an observer,
> doesn't it?  The standard case of drawImage() tends to hand in the
> Component as the observer.  In cases where we know we are using a
> BufferedImage, we often use null, but code that uses a toolkit image (or
> that doesn't know what the image is) should be using a non-null observer
> or it won't paint right the first time when it triggers the image loading.
>
> The cache object itself takes so little room that I don't think it is
> worth worrying about.  If something triggers MRToolkitImage to be
> referenced at all, then the 99% case will likely eventually involve
> drawImage() calls with observer and an empty cache takes so little room
> that it is probably fine to just aggressively create it at that time.
>
> There is no bug or problem here, I just don't think the indirection buys
> us anything worthwhile here...
>
>              ...jim