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

[Alexander Scherbatiy]

   Could you review the updated fix:
     http://cr.openjdk.java.net/~alexsch/8011059/webrev.14
   - Observer and rvObserver are used to get width/hight from 
image/rvImage in SunGraphics2D
   - width and height are rounded up in the image observer wrapper
   - width and height are also rescaled for the SOMEBITS, FRAMEBITS , 
and ALLBITS infoflags

  Thanks,
  Alexandr.


On 12/3/2013 9:49 PM, 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