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

[Phil Race]

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:_
>
> http://cr.openjdk.java.net/~jdv/8143562/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ejdv/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; 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 : http://cr.openjdk.java.net/~jdv/8143562/webrev.00/
>         <http://cr.openjdk.java.net/%7Ejdv/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/20151215/fb474a98/attachment.html>