Thanks for the review Phil. Can I get one more review for new webrev : HYPERLINK "http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/"http://cr.openjdk.java.net/~jdv/8143562/webrev.01/
Regards, Jay From: Phil Race Sent: Wednesday, December 16, 2015 12:00 AM To: Jayathirth D V Cc: 2d-dev@openjdk.java.net; brian Burkhalter Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns null for getRawImageType() Looks OK. -phil. On 12/14/2015 01:37 AM, Jayathirth D V wrote: Hi Phil, I have made changes based on your suggestions. I have removed specific comment which mentioned about how we handle YCbCr in JPEG and just added common comment(If there is no close match then a type which preserves the most information from the image should be returned) in ImageReader.java. Please find the updated webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.01/"http://cr.openjdk.java.net/~jdv/8143562/webrev.01/ After technical review is done I will raise CCC request since there is spec change in public API. Please review. Thanks, Jay From: Philip Race Sent: Friday, December 11, 2015 2:58 AM To: prasanta sadhukhan Cc: Jayathirth D V; HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns null for getRawImageType() The quoted code comment is essentially disclaiming the spec comment. And the spec. comment is misleading in that it strongly suggests getRawImageType will hand you an ImageTypeSpecifier representing YCbCr when in fact it does not. In fact all these comments and behaviours together seem to suggest that there isn't a consistent view of what should happen here. - one says we can return something that represents YCbCr - another says don't add it to the returned list of types because we can't support this raw type after all. - the spec says return something (anything!) no matter how dissimilar (implied by there being at least one (a non-null return)). Basically that seems to be all the options except supporting it. All of these need to line up and agree. So I think we need to update the spec. below to say something like "Returns an ImageTypeSpecifier indicating the SampleModel and ColorModel which most closely represents the "raw" internal format of the image. If there is no close match then a type which preserves the most information from the image should be returned." -phil On 12/8/15, 11:31 PM, prasanta sadhukhan wrote: The fix looks good to me. The spec says "Returns an ImageTypeSpecifier indicating the SampleModel and ColorModel which most closely represents the "raw" internal format of the image. For example, for a JPEG image the raw type might have a YCbCr color space even though the image would conventionally be transformed into an RGB color space prior to display." Also, private Iterator<ImageTypeSpecifier> getImageTypesOnThread(int imageIndex)adds RGB for YcbCr raw type case JPEG.JCS_YCbCr: 832 // As there is no YCbCr ColorSpace, we can't support 833 // the raw type. 834 835 // due to 4705399, use RGB as default in order to avoid 836 // slowing down of drawing operations with result image. 837 list.add(getImageType(JPEG.JCS_RGB)); Regards Prasanta On 12/1/2015 3:42 PM, Jayathirth D V wrote: Hi, Please review following fix in JDK9: Bug : https://bugs.openjdk.java.net/browse/JDK-8143562 Webrev : HYPERLINK "http://cr.openjdk.java.net/%7Ejdv/8143562/webrev.00/"http://cr.openjdk.java.net/~jdv/8143562/webrev.00/ Issue : We are getting null for ImageTypeSpecifier when we use getRawImageType() API for YCbCr Image. Root cause : When colorspace is YCbCr, there is no condition to create ImageTypeSpecifier in produce() function Solution : Since we add RGB as default ImageType for YCbCr colorspace in getImageTypes() API. Followed same approach and considering it as RGB in getRawImageType(). Thanks, Jay
