Re: [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@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