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

[Jim Graham]

Hi Alexander,

It looks good to go.  I only skimmed the other parts of the fix on the 
assumption that they haven't changed in a few revisions, but it all 
looked good.  Glad to see you fixed the dimension issues in the observer 
as well.

I'll follow up with a list of future issues to track...

			...jim

On 12/4/13 6:15 AM, Alexander Scherbatiy wrote:
>
>    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
>