Re: [OpenJDK 2D-Dev] RFR: 7183828: Invalid Image Variant when using anything other than BufferedImage
On Tue, 8 Sep 2020 21:16:10 GMT, Sergey Bylokhov wrote: > It is intended that our draw machinery works only on specific image formats > that we know we support. > But the user still able to create some image subclasses for their purpose and > according to our spec of > Graphics2D.drawImage() we should not throw any exceptions. > > I suggest handling this situation in a similar way as we handle some errors > during rendering when > the pipeline is in the wrong state, or the ToolkitImage is not loaded yet, > just ignore the request and > return false. > > All our pipelines have a special meaning of InvalidPipeException, if the > pipeline found that it cannot complete the draw > operation throws this exception which is handled by all methods in the > SunGraphics2D class. > > So as a fix I suggest changing the IllegalArgumentException to the > InvalidPipeException. > Also, we need to add a try/catch block to the drawHiDPIImage(it uses the > SurfaceManager.getManager method directly) > > > Old review request: > https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/010995.html Ok. Fair enough. Please copy those spec. excerpts into the bug report. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/85
Re: [OpenJDK 2D-Dev] RFR: 7183828: Invalid Image Variant when using anything other than BufferedImage
On Wed, 9 Sep 2020 16:50:22 GMT, Phil Race wrote:
> I have very mixed feelings about this whole idea.
> InvalidPipeException is being co-opted for a different purpose.
We already use this exception in such cases, and I think it is intended for
this:
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/java2d/opengl/OGLMaskFill.java#L75
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/java2d/NullSurfaceData.java#L92
> The user no longer gets a clear message that their image type isn't supported.
> Is there any specification we can point to ?
The spec for the Image class:
> * The abstract class {@code Image} is the superclass of all
> * classes that represent graphical images. The image must be
> * obtained in a platform-specific manner.
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/Image.java#L40
Or the spec for the VolatileImage
> * This image should not be subclassed directly but should be created
> * by using the {@link java.awt.Component#createVolatileImage(int, int)
> * Component.createVolatileImage} or
> * {@link java.awt.GraphicsConfiguration#createCompatibleVolatileImage(int,
> int)
> * GraphicsConfiguration.createCompatibleVolatileImage(int, int)} methods.
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/image/VolatileImage.java#L74
-
PR: https://git.openjdk.java.net/jdk/pull/85
Re: [OpenJDK 2D-Dev] RFR: 7183828: Invalid Image Variant when using anything other than BufferedImage
On Tue, 8 Sep 2020 21:16:10 GMT, Sergey Bylokhov wrote: > It is intended that our draw machinery works only on specific image formats > that we know we support. > But the user still able to create some image subclasses for their purpose and > according to our spec of > Graphics2D.drawImage() we should not throw any exceptions. > > I suggest handling this situation in a similar way as we handle some errors > during rendering when > the pipeline is in the wrong state, or the ToolkitImage is not loaded yet, > just ignore the request and > return false. > > All our pipelines have a special meaning of InvalidPipeException, if the > pipeline found that it cannot complete the draw > operation throws this exception which is handled by all methods in the > SunGraphics2D class. > > So as a fix I suggest changing the IllegalArgumentException to the > InvalidPipeException. > Also, we need to add a try/catch block to the drawHiDPIImage(it uses the > SurfaceManager.getManager method directly) > > > Old review request: > https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/010995.html I have very mixed feelings about this whole idea. InvalidPipeException is being co-opted for a different purpose. The user no longer gets a clear message that their image type isn't supported. Is there any specification we can point to ? - PR: https://git.openjdk.java.net/jdk/pull/85
[OpenJDK 2D-Dev] RFR: 7183828: Invalid Image Variant when using anything other than BufferedImage
It is intended that our draw machinery works only on specific image formats that we know we support. But the user still able to create some image subclasses for their purpose and according to our spec of Graphics2D.drawImage() we should not throw any exceptions. I suggest handling this situation in a similar way as we handle some errors during rendering when the pipeline is in the wrong state, or the ToolkitImage is not loaded yet, just ignore the request and return false. All our pipelines have a special meaning of InvalidPipeException, if the pipeline found that it cannot complete the draw operation throws this exception which is handled by all methods in the SunGraphics2D class. So as a fix I suggest changing the IllegalArgumentException to the InvalidPipeException. Also, we need to add a try/catch block to the drawHiDPIImage(it uses the SurfaceManager.getManager method directly) Old review request: https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/010995.html - Commit messages: - 7183828: Invalid Image Variant when using anything other than BufferedImage Changes: https://git.openjdk.java.net/jdk/pull/85/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=85=00 Issue: https://bugs.openjdk.java.net/browse/JDK-7183828 Stats: 413 lines in 3 files changed: 268 ins; 100 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/85.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/85/head:pull/85 PR: https://git.openjdk.java.net/jdk/pull/85
Re: [OpenJDK 2D-Dev] RFR: 7183828 Invalid Image Variant when using anything other than BufferedImage
Hi, Phil. Also now we have more checks for specific known image types. VolatileImage is an abstract class and I'm surprised that this is something this fix considers OK for subclasses to extend. Does that really work ? This is a good question I'll double-check that. You are right, we do not support the abstract VolatileImage class as well, and it is not possible to fix this in a similar way as I did in the first revision, because we get this exception for VolatileImage much early. So I tried to fix it from the other way around. All our pipelines have a special meaning of InvalidPipeException, if the pipeline found that it cannot complete draw operation it throws this exception which is handled by all methods in the SunGraphics2D class. So as a fix I suggest to change the IllegalArgumentException to the InvalidPipeException. Also we need to add try/catch block to the drawHiDPIImage(it uses the SurfaceManager.getManager method directly) An updated version: http://cr.openjdk.java.net/~serb/7183828/webrev.01 -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR: 7183828 Invalid Image Variant when using anything other than BufferedImage
On 26.08.2020 11:27, Philip Race wrote: Can you please add an evaluation to the bug report explaining what you intend to do. Will do. The submitter of that bug was clearly hoping that we'd support custom Image subclasses and this fix just changes the behaviour from an IAE (or similar) to silently ignoring them. I'm not sure what benefit there is there. It just means that people will be more puzzled as to what is going on than before. After the fix, we will start to support them to some degree. The drawImage spec does not have any exceptions, this method can only report is the pixels were completly pushed to the destination or nor. After the fix we will report "false" for any unsupported types of images. The new behavior will be similar to the ToolkitImage case, when the image is not loaded yet. Also now we have more checks for specific known image types. VolatileImage is an abstract class and I'm surprised that this is something this fix considers OK for subclasses to extend. Does that really work ? This is a good question I'll double-check that. I think we should have just closed this out as WNF / not an issue. -phil. On 7/27/20, 10:14 PM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk/client. Bug: https://bugs.openjdk.java.net/browse/JDK-7183828 Fix: http://cr.openjdk.java.net/~serb/7183828/webrev.00 Our DrawImage pipe, used as low-level machinery to draw the known type of images, supports only three types of images: - ToolkitImage - implemented via ImageRepresentation.drawToBufImage() - VolatileImage/BufferedImage implemented via different types of "loops" We have a type check for the ToolkitImage image only, otherwise, we assume that the image is of type VolatileImage/BufferedImage, so if the user creates its own image and passes it to this pipe he will get an exception. After the fix, such custom images will be ignored by the DrawImage pipe. -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR: 7183828 Invalid Image Variant when using anything other than BufferedImage
Can you please add an evaluation to the bug report explaining what you intend to do. The submitter of that bug was clearly hoping that we'd support custom Image subclasses and this fix just changes the behaviour from an IAE (or similar) to silently ignoring them. I'm not sure what benefit there is there. It just means that people will be more puzzled as to what is going on than before. Also now we have more checks for specific known image types. VolatileImage is an abstract class and I'm surprised that this is something this fix considers OK for subclasses to extend. Does that really work ? I think we should have just closed this out as WNF / not an issue. -phil. On 7/27/20, 10:14 PM, Sergey Bylokhov wrote: Hello. Please review the fix for jdk/client. Bug: https://bugs.openjdk.java.net/browse/JDK-7183828 Fix: http://cr.openjdk.java.net/~serb/7183828/webrev.00 Our DrawImage pipe, used as low-level machinery to draw the known type of images, supports only three types of images: - ToolkitImage - implemented via ImageRepresentation.drawToBufImage() - VolatileImage/BufferedImage implemented via different types of "loops" We have a type check for the ToolkitImage image only, otherwise, we assume that the image is of type VolatileImage/BufferedImage, so if the user creates its own image and passes it to this pipe he will get an exception. After the fix, such custom images will be ignored by the DrawImage pipe.
[OpenJDK 2D-Dev] RFR: 7183828 Invalid Image Variant when using anything other than BufferedImage
Hello. Please review the fix for jdk/client. Bug: https://bugs.openjdk.java.net/browse/JDK-7183828 Fix: http://cr.openjdk.java.net/~serb/7183828/webrev.00 Our DrawImage pipe, used as low-level machinery to draw the known type of images, supports only three types of images: - ToolkitImage - implemented via ImageRepresentation.drawToBufImage() - VolatileImage/BufferedImage implemented via different types of "loops" We have a type check for the ToolkitImage image only, otherwise, we assume that the image is of type VolatileImage/BufferedImage, so if the user creates its own image and passes it to this pipe he will get an exception. After the fix, such custom images will be ignored by the DrawImage pipe. -- Best regards, Sergey.
