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
im...@2x.png
files on the disk.
Image image1 = Toolkit.getImage("image.png"); // load as
multi-resolution image
Image image2 = Toolkit.getImage("im...@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
