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

[Jim Graham]

Ah, sorry, one last (non-blocking) issue with the new code.  Typically 
we report image loading a scan line at a time for GIFs and 
non-progressive JPEGs.  The simple "divide by integer 2" lines in the 
new observer will round OK for the x,y, but the dimensions will be 
rounded down and often be wrong.  For now I think this is OK since I'm 
not sure that anyone actually uses the notifications of single scan 
lines to do anything but redraw the image in its entirety, but it should 
go on the list of things to fix later.

Minimally, you could change the lines that adjust the dimensions to 
"(width + 1) / 2" and "(height + 1) / 2" which isn't quite correct, but 
should be reasonably close for now.  Eventually we should be using 
"round down, round up" semantics on the full rectangle, but for the 
simple case of 2x, this simple change should be enough...

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