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

[Sergey Bylokhov]

Hi, Alexander.
Count me as a second reviewer.

On 12/4/13 10:16 PM, Jim Graham wrote:
> 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
>>


-- 
Best regards, Sergey.