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

[Jim Graham]

(Non-blocking issue discussion here...)

Hmmm...  This won't actually help anyway because you only divide the 
dimensions when the WIDTH/HEIGHT flags are set.  The tests for dividing 
the dimensions should also include SOMEBITS, FRAMEBITS, and ALLBITS 
since all 3 of those include a scaled width/height.

Again, this issue isn't blocking because at least the WIDTH|HEIGHT flags 
get the correct information and all of the other flags which report 
which part of the image changed, will just over-report the dimensions of 
the new data (for now)...

			...jim

On 12/3/13 10:26 AM, Jim Graham wrote:
> 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