Just to be specific so we don't get bogged down in misunderstandings.
Here is the modification that I'm suggesting:
3086 } else if (img instanceof MultiResolutionImage) {
3087 // get scaled destination image size
3088
3089 int width = img.getWidth(null);
3090 int height = img.getHeight(null);
3091
3092 Image resolutionVariant = getResolutionVariant(
3093 (MultiResolutionImage) img, width, height,
3094 dx1, dy1, dx2, dy2, sx1, sy1, sx2, sy2);
3095
3096 if (resolutionVariant != img && resolutionVariant !=
null) {
3097 // recalculate source region for the resolution variant
3098
ImageObserver rvobserver =
MultiResolutionToolkitImage.
getResolutionVariantObserver(img,
observer,
width, height, -1, -1);
3099 int rvWidth = resolutionVariant.getWidth(rvobserver);
3100 int rvHeight = resolutionVariant.getHeight(rvobserver);
3101
3102 if (0 < width && 0 < height && 0 < rvWidth && 0 <
rvHeight) {
3103
3104 float widthScale = ((float) rvWidth) / width;
3105 float heightScale = ((float) rvHeight) / height;
3106
3107 sx1 = Region.clipScale(sx1, widthScale);
3108 sy1 = Region.clipScale(sy1, heightScale);
3109 sx2 = Region.clipScale(sx2, widthScale);
3110 sy2 = Region.clipScale(sy2, heightScale);
3111
observer = rvobserver;
3115 img = resolutionVariant;
3116 }
3117 }
3118 }
(And perhaps this explains why I was pushing for the rv dimensions to be
determined on the fly - or hardcoded at 2x for now - in the wrapped
observer?)
...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 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
