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

[Jim Graham]

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