Re: [OpenJDK 2D-Dev] [9] RFR JDK-4924727 : reader.abort() method does not work when called inside imageStarted for PNG
Is it possible to unify the test for all our plugins? I assume they should work in the same way. I am not sure but probably the image can be generated at runtime? On 11.08.16 21:59, Jayathirth D V wrote: Hi, Please review the following fix in JDK9 at your convenience: Bug : https://bugs.openjdk.java.net/browse/JDK-4924727 Webrev : http://cr.openjdk.java.net/~jdv/4924727/webrev.00/ Issue : When we issue ImageReader.abort() in IIOReadProgressListener.imageStarted(), reading is not aborted and it is continued. Root cause : After IIOReadProgressListener.imageStarted() call in PNGImageReader.java when we enter decodeImage() we call clearAbortRequest() which will clear the abort request from IIOReadProgressListener.imageStarted(). Solution : clearAbortRequest() documentation mentions that it should be called before reading of image starts, so it should be called before IIOReadProgressListener.imageStarted()(In PNGImageReader.java it is processImageStarted(0) in readImage()). So moved clearAbortRequest() call from decodeImage() to readImage(). Also we should call abortRequested() in PNGImageReader.java at places mapping to IIOReadProgressListener and not randomly at end of functions or at places related to IIOReadUpdateListener, updated the code for the same. Observation not related to this issue : We don’t have call similar to IIOReadProgressListener.readAborted() in IIOReadUpdateListener, but user can call ImageReader.abort() from IIOReadUpdateListener methods. Is there a need to add similar method in IIOReadUpdateListener? Any inputs on this also would be helpful. Thanks, Jay -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [9] Review request for 8151303 [macosx] [hidpi] JButton's low-res. icon is visible when clicking on it
Looks good. +1 ...jim On 08/15/2016 08:47 AM, Alexander Scherbatiy wrote: Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.04 The MRCI.sizes arrays is reused for for MultiResolutionCachedImage. Thanks, Alexandr. On 11/08/16 23:10, Jim Graham wrote: Hi Alexandr, Should something be done to transfer the array of sizes to the new instance if the source is an MRCI? Perhaps a special case for MRCI as well that calls mrciInstance.map(mapper) instead of constructing a brand new object from scratch? ...jim On 08/11/2016 01:32 AM, Alexander Scherbatiy wrote: Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.03 MultiResolutionToolkitImage handing is added to the MultiResolutionCachedImage.map() method. Thanks, Alexandr. On 11/08/16 01:46, Jim Graham wrote: Ah, yes, only ToolkitImages can be "not yet loaded" in that manner. A quick look suggests that a MRCI should not be an instance of MRTI, but MRCI.map() does not force its argument to be an instance of MRCI, just MRI, so it would be possible for someone to pass an MRTI to MRCI.map() and then it would have this problem. Should we change the argument of MRCI.map() to MRCI? (And then you don't need to cast to Image and use getWidth/Height(Observer) to get its dimensions.) If that is not feasible, we should have it do something different for a non-MRCI... ...jim On 8/10/16 5:35 AM, Alexander Scherbatiy wrote: On 09/08/16 03:49, Jim Graham wrote: Does MultiResolutionCachedImage.map() work if the Image hasn't been loaded yet (where getWidth/Height(Observer) can return -1)? Can it ever be called in a case like that? Could we rely on the fact that getWidth/Height(Observer) returns -1 only for ToolkitImage? If so, the passed MultiResolutionToolkitImage is handled in MultiResolutionToolkitImage.map() method. If no, the fix should be updated to load the image size in some way. Thanks, Alexandr. ...jim On 08/08/2016 12:48 PM, Alexander Scherbatiy wrote: Just a friendly reminder. Thanks, Alexandr. On 27/06/16 22:17, Alexander Scherbatiy wrote: Hello, Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.02 The fix does not use a new public API to apply filters to multi-resolution images. Thanks, Alexandr. On 14/05/16 02:54, Jim Graham wrote: Another reason to avoid new API is that we don't have to involve the CCC to get this "bug fix" in... ...jim On 5/13/16 3:50 PM, Jim Graham wrote: That looks very tight. The only issue I'd have is that it would be better if this could be done with non-public API for now - the map() methods could live on one of the sun.awt.image classes or even in a Swing implementation utility class and still work just fine. When we have more time to figure out how we're going to handle filtering of MRIs in general we can decide if this API should live on the base MRI interface. The only thing we'd lose is BaseMRI having an optimized implementation of the map() method, but I don't think it's implementation represents enough of an optimization to matter when we are creating and loading dozens of images... ...jim On 5/12/16 10:08 AM, Alexander Scherbatiy wrote: Hello, Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.01 There was a suggestion to postpone the fix for the issue 8152309 Seamless way of using image filters with multi-resolution images and continue with the current one: http://mail.openjdk.java.net/pipermail/2d-dev/2016-April/006766.html The new version of the fix introduces a mapper method which allows to map all resolution variants of one multi-resolution image to another: Image image = // original image Image filteredImage = MultiResolutionImage.map(image, (img) -> /* return filtered image */); Thanks, Alexandr. On 21/03/16 22:31, Jim Graham wrote: We could do that for our own filters, but any random custom filter could run into the same issue, so it might not make sense to upgrade the existing filter mechanism to always attempt to do MR filtering. We could either create a parallel set of MR-aware filter mechanisms (such as the previously suggested new method on Toolkit, or a new MR-aware version of FilteredImageSource for instance) and leave the existing mechanism as clearly documented as MR-unaware. Another idea is to tag a filter with an interface that indicates that it is MR aware? In any case, some thought needs to be given to feeding an MR image to a filter that (potentially or demonstrably) cannot deal with MR images. Alternately, we could then recommend that the old image filtering code isn't combined with multi-resolution images. It seems to me that the programmer is mostly in control over this happening since they've either manually created the MR image using the custiom MR image mechanism or they've
Re: [OpenJDK 2D-Dev] [9] Review request for 8151303 [macosx] [hidpi] JButton's low-res. icon is visible when clicking on it
Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.04 The MRCI.sizes arrays is reused for for MultiResolutionCachedImage. Thanks, Alexandr. On 11/08/16 23:10, Jim Graham wrote: Hi Alexandr, Should something be done to transfer the array of sizes to the new instance if the source is an MRCI? Perhaps a special case for MRCI as well that calls mrciInstance.map(mapper) instead of constructing a brand new object from scratch? ...jim On 08/11/2016 01:32 AM, Alexander Scherbatiy wrote: Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.03 MultiResolutionToolkitImage handing is added to the MultiResolutionCachedImage.map() method. Thanks, Alexandr. On 11/08/16 01:46, Jim Graham wrote: Ah, yes, only ToolkitImages can be "not yet loaded" in that manner. A quick look suggests that a MRCI should not be an instance of MRTI, but MRCI.map() does not force its argument to be an instance of MRCI, just MRI, so it would be possible for someone to pass an MRTI to MRCI.map() and then it would have this problem. Should we change the argument of MRCI.map() to MRCI? (And then you don't need to cast to Image and use getWidth/Height(Observer) to get its dimensions.) If that is not feasible, we should have it do something different for a non-MRCI... ...jim On 8/10/16 5:35 AM, Alexander Scherbatiy wrote: On 09/08/16 03:49, Jim Graham wrote: Does MultiResolutionCachedImage.map() work if the Image hasn't been loaded yet (where getWidth/Height(Observer) can return -1)? Can it ever be called in a case like that? Could we rely on the fact that getWidth/Height(Observer) returns -1 only for ToolkitImage? If so, the passed MultiResolutionToolkitImage is handled in MultiResolutionToolkitImage.map() method. If no, the fix should be updated to load the image size in some way. Thanks, Alexandr. ...jim On 08/08/2016 12:48 PM, Alexander Scherbatiy wrote: Just a friendly reminder. Thanks, Alexandr. On 27/06/16 22:17, Alexander Scherbatiy wrote: Hello, Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.02 The fix does not use a new public API to apply filters to multi-resolution images. Thanks, Alexandr. On 14/05/16 02:54, Jim Graham wrote: Another reason to avoid new API is that we don't have to involve the CCC to get this "bug fix" in... ...jim On 5/13/16 3:50 PM, Jim Graham wrote: That looks very tight. The only issue I'd have is that it would be better if this could be done with non-public API for now - the map() methods could live on one of the sun.awt.image classes or even in a Swing implementation utility class and still work just fine. When we have more time to figure out how we're going to handle filtering of MRIs in general we can decide if this API should live on the base MRI interface. The only thing we'd lose is BaseMRI having an optimized implementation of the map() method, but I don't think it's implementation represents enough of an optimization to matter when we are creating and loading dozens of images... ...jim On 5/12/16 10:08 AM, Alexander Scherbatiy wrote: Hello, Could you review the updated fix: http://cr.openjdk.java.net/~alexsch/8151303/webrev.01 There was a suggestion to postpone the fix for the issue 8152309 Seamless way of using image filters with multi-resolution images and continue with the current one: http://mail.openjdk.java.net/pipermail/2d-dev/2016-April/006766.html The new version of the fix introduces a mapper method which allows to map all resolution variants of one multi-resolution image to another: Image image = // original image Image filteredImage = MultiResolutionImage.map(image, (img) -> /* return filtered image */); Thanks, Alexandr. On 21/03/16 22:31, Jim Graham wrote: We could do that for our own filters, but any random custom filter could run into the same issue, so it might not make sense to upgrade the existing filter mechanism to always attempt to do MR filtering. We could either create a parallel set of MR-aware filter mechanisms (such as the previously suggested new method on Toolkit, or a new MR-aware version of FilteredImageSource for instance) and leave the existing mechanism as clearly documented as MR-unaware. Another idea is to tag a filter with an interface that indicates that it is MR aware? In any case, some thought needs to be given to feeding an MR image to a filter that (potentially or demonstrably) cannot deal with MR images. Alternately, we could then recommend that the old image filtering code isn't combined with multi-resolution images. It seems to me that the programmer is mostly in control over this happening since they've either manually created the MR image using the custiom MR image mechanism or they've supplied media with multiple resolution files (i.e. "@2x"). Is that really the
