[OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-8143562: JPEG reader returns null for getRawImageType()

[Jayathirth D V]

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 at 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 at openjdk.java.net"2d-dev at 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

 

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20151216/365af870/attachment.html>