[OpenJDK 2D-Dev] Review Request for JDK-8139183 : drawImage misses background's alpha channel

[Jayathirth D V]

Hi,

 

I have updated the changes to select proper Buffer Image type based on source 
transparency and not just using ARGB directly.

 

Please find the updated webrev for review:

http://cr.openjdk.java.net/~jdv/8139183/webrev.01/ 

 

Thanks,

Jay

 

From: Jayathirth D V 
Sent: Wednesday, March 02, 2016 5:02 PM
To: 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan
Subject: Review Request for JDK-8139183 : drawImage misses background's alpha 
channel

 

Hi,

 

Please review the following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8139183 

 

Webrev : http://cr.openjdk.java.net/~jdv/8139183/webrev.00/ 

 

Issue : When we scale any buffered image using drawImage() API which takes 
scale coordinates we are losing alpha channel in background color.

 

Root cause : We are creating opaque temporary image when we have background 
color and scale is happening in renderImageXform() API of DrawImage.java. By 
making it opaque we are losing translucency.

 

Solution : Instead of creating opaque RGB temporary image use ARGB temporary 
image to maintain translucency of image.

 

Thanks,

Jay

[OpenJDK 2D-Dev] Review Request for JDK-8139183 : drawImage misses background's alpha channel

[Jayathirth D V]

Hi,

 

Please review the following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8139183 

 

Webrev : http://cr.openjdk.java.net/~jdv/8139183/webrev.00/ 

 

Issue : When we scale any buffered image using drawImage() API which takes 
scale coordinates we are losing alpha channel in background color.

 

Root cause : We are creating opaque temporary image when we have background 
color and scale is happening in renderImageXform() API of DrawImage.java. By 
making it opaque we are losing translucency.

 

Solution : Instead of creating opaque RGB temporary image use ARGB temporary 
image to maintain translucency of image.

 

Thanks,

Jay

Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED

[Jayathirth D V]

Hi Jim,

Gentle reminder. Please let me your inputs after testing.

Thanks,
Jay

-Original Message-
From: Jayathirth D V 
Sent: Tuesday, February 16, 2016 10:03 PM
To: Jim Graham
Cc: 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan
Subject: RE: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel 
colour when converting images to TYPE_BYTE_INDEXED

Hi Jim,

Thanks for letting me know about the importance of custom color maps. I was not 
able to understand what you mentioned previously since I am new to this 
terminology.

Regarding performance I checked with image having complete 255,255,128 pixels 
and ran it for 100 times. I am running on Windows 7  Professional SP1 with 
Intel i5-5300U CPU. Overall I don't see much difference in time taken for 
drawImage() API before and after change. Details for INT_RGB image as input:

Before change :
Minimum time   536827ns
Maximum time  1560947ns
Average time  871167ns

After change :
Minimum time  536380ns
Maximum time 1468130ns
Average time 830778ns

There is +/- 10% delta both for before and after change time but there is no 
major gap. Even for image with other input type it holds the same.


Regarding custom color maps, previously I ran on default color map generated in 
TYPE_BYTE_INDEXED constructor. Now I tested with modified color map so that it 
doesn't contain any of the primary color values. Instead of using 0-255 values 
with value 51 increments in R,G,B components I used color map with  20-245 
values with value 45 as increment. For this color map before and after change 
all the pixels are referring to same index in color map irrespective of removal 
of error delta addition in case of primary colors.
 
I have attached log for Red and Green primary colors before and after 
change(Was not able to attach for more since it exceeded 120KB mailing list 
limit). Please search for "Final index value =" in the logs. Please let me know 
your inputs.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Tuesday, February 16, 2016 1:03 AM
To: Jayathirth D V
Cc: 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel 
colour when converting images to TYPE_BYTE_INDEXED

Hi Jayathirth,

The issue with testing performance with white images is that the tests allow 
the code to eliminate 3 indexed fetches which take time.  Primary colors end up 
being the one obscure case where the new code might be faster.  But, 
non-primary colors would be slower by a fair amount, so performance testing 
with converting a randomized or non-primary-color image are the more telling 
case.  An image filled with 255,255,128 would be the worst case because it 
would involve all of the tests and still have to do all of the lookups.

My question about how this works with custom colormaps was not really addressed 
by your response.  A simple test to make sure the colormap has accurate (or 
very close) conversions for the primary colors may be enough to validate the 
colormap for the indicated tests.  If the colormap contains no pure conversions 
for some of the primary colors then they may need to be dithered anyway...

...jim

On 2/15/16 3:39 AM, Jayathirth D V wrote:
> Hi Jim,
>
> I performed performance analysis with white image so that all 
> conditions are checked in the new branch added. There is no major 
> difference in time taken before and after change. For each input I 
> have tested time taken for drawImage() API to convert from every 
> format to Byte indexed type. For every unique conversion test is run 
> for 100 times. Please find the details:
>
> Input
>
> type
>
>   
>
> Min
>
> before change
>
> (ns)
>
>   
>
> Min
>
> after change
>
> (ns)
>
>   
>
> Max
>
> before change
>
> (ns)
>
>   
>
> Max
>
> after change
>
> (ns)
>
>   
>
> Average
>
> before change
>
> (ns)
>
>   
>
> Average
>
> after
>
> change
>
> (ns)
>
> INT_RGB
>
>   
>
> 523437
>
>   
>
> 481491
>
>   
>
> 1230724
>
>   
>
> 1270440
>
>   
>
> 789452
>
>   
>
> 666144
>
> INT_ARGB
>
>   
>
> 500232
>
>   
>
> 493986
>
>   
>
> 12406307
>
>   
>
> 1308816
>
>   
>
> 793384
>
>   
>
> 654015
>
> INT_ARGB_PRE
>
>   
>
> 500233
>
>   
>
> 492201
>
>   
>
> 1037057
>
>   
>
> 981277
>
>   
>
> 710250
>
>   
>
> 699214
>
> INT_BGR
>
>   
>
> 537716
>
>   
>
> 562706
>
>   
>
> 1446703

Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED

[Jayathirth D V]

Hi Jim,

 

I performed performance analysis with white image so that all conditions are 
checked in the new branch added. There is no major difference in time taken 
before and after change. For each input I have tested time taken for 
drawImage() API to convert from every format to Byte indexed type. For every 
unique conversion test is run for 100 times. Please find the details:

 

 

Input

type

Min

before change

(ns)

Min

after change

(ns)

Max

before change

(ns)

Max

after change

(ns)

Average

before change

(ns)

Average

after

change

(ns)

INT_RGB

523437

481491

1230724

1270440

789452

666144

INT_ARGB

500232

493986

12406307

1308816

793384

654015

INT_ARGB_PRE

500233

492201

1037057

981277

710250

699214

INT_BGR

537716

562706

1446703

2046001

862377

863256

3BYTE_BGR

483275

481045

1181638

1384676

651427

580961

4BYTE_ABGR

544410

499786

1292305

968783

690106

683881

4BYTE_ABGR_PRE

504249

505588

1680086

1216445

756101

687750

USHORT_565_RGB

507818

505588

978153

1346746

652908

655782

USHORT_555_RGB

510496

509604

952272

1162004

650418

670811

BYTE_GRAY

481491

478367

1140585

1799231

691160

583250

USHORT_GRAY

506927

507373

1375751

1255267

728202

646902

BYTE_BINARY

541733

496217

1083466

959411

730527

728461

 

 

The changes are tested with plain images having primary colors like RED, GREEN, 
BLUE, BLACK and WHITE.

 

Thanks,

Jay

 

-Original Message-
From: Jim Graham 
Sent: Friday, February 12, 2016 4:05 AM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel 
colour when converting images to TYPE_BYTE_INDEXED

 

Hi Jayathirth,

 

Did you do any performance analysis of this change?  You are adding 6 tests and 
multiple branches to per-pixel code.

 

The effectiveness of this technique depends on the colormap that we have set 
up.  For the BufferedImage.TYPE_INDEXED constructor we produce a fairly nice 
colormap, but if someone creates a custom colormap then the colors could be 
anything.  We create a decent inversion for just about any colormap, but that 
doesn't mean that using only "the best match for solid red" will produce a 
better result for a dithered approximation for red.  It is true that if there 
is a really good match for red then we should just use that, but if there are 
no direct matches for red then we may choose to paint a red region with solid 
orange even though there is another color in the colormap that when mixed with 
orange approximates a red tone better.  For example, if a colormap contains no 
pure red, but

contains:

 

240, 20,  0

240,  0, 20

 

(I'm not sure if 20 is within our current error deltas that we use for 
dithering, but this is an example not a test case.)

 

Then using one of these alone might skew the color towards orange or purple.  
Using both together in a dither pattern might keep the overall hue impression 
as red, but with a small amount of noise in its saturation.

 

What types of colormaps was this tested with?

 

...jim

 

On 2/11/16 6:37 AM, Jayathirth D V wrote:

> Hi,

> 

> _Please review the following fix in JDK9:_

> 

> __

> 

> Bug : https://bugs.openjdk.java.net/browse/JDK-7116979

> 

> Webrev : http://cr.openjdk.java.net/~jdv/7116979/webrev.00/

> 

> Issue : When Image containing black pixels are converted from any 

> format to Byte Indexed format some of the pixels are not black. They 

> are following pattern similar to dithering.

> 

> Root cause : When we convert any format type to ByteIndexed we are 

> adding Error delta values to R,G,B components using dithering indices.

> This is causing some pixels values to not point to proper index in 

> color table.

> 

> Solution : There is no need to add error delta for primary colors 

> containing basic values in R,G,B components. Exclude such pixels from 

> delta addition.

> 

> Thanks,

> 

> Jay

> 

 

Re: [OpenJDK 2D-Dev] Review request for JDK-8147413 : api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE] started to fail

[Jayathirth D V]

Hi Alex,

Since two separate bugs are created for the suggestions given by Jim.
And you are okay with the fix provided for the bug 
https://bugs.openjdk.java.net/browse/JDK-8147413 under webrev 
http://cr.openjdk.java.net/~jdv/8147413/webrev.00/ 

Can we push the fix into JDK9 after one more approval?

Thanks,
Jay

-Original Message-
From: Alexander Scherbatiy 
Sent: Friday, January 22, 2016 5:05 PM
To: Jim Graham; Phil Race; Jayathirth D V
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review request for JDK-8147413 : 
api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE]
 started to fail


I have created two separated issues on it:
   8148045 Handle null resolution variant in SunGraphics2D
 https://bugs.openjdk.java.net/browse/JDK-8148045

   8148046 Investigate the case where MRI.getResolutionVariant() returns 
MultiResolutionImage
 https://bugs.openjdk.java.net/browse/JDK-8148046

Thanks,
Alexandr.

On 22/01/16 00:34, Jim Graham wrote:
> BASE essentially just means to draw the 1:1 image.  The problem is 
> that returning null is not the way to achieve that.  Returning null 
> means "MRI doesn't apply here, just use the original image", but in 
> the case of BASE we have to make sure it isn't an MRI first otherwise 
> the caller will get an exception.
>
> The change does achieve that since BASE handling is built in to the 
> helper getResolutionVariant() method in SG2D and so it should ask for 
> the 1:1 image, but if the getRV() helper method returns null then we 
> still return null from the drawHiDPIImage() method and that causes the 
> caller to use the original MRI in a normal-image pipeline.  There 
> should perhaps be an else clause for the "if (rV != img && rV != null) 
> { } else { ???; }". What that "???" is depends on why the helper 
> method returned the original image and/or a null, but it should not be 
> "return null;". (Also, there is no check if it returned another 
> different MRI. That should probably not be allowed, but we never state 
> that in the interface description...)
>
> The remaining question is why the helper method would return either 
> the original image or null.
>
> I don't think the helper method ever returns the original MRI 
> knowingly, but the MRI itself might do that.  That would then fall 
> under the "what to do if the MRI returns any sort of non-standard 
> image" case, and perhaps the exception is appropriate there?  We 
> should probably detect that, though, and throw a more obvious 
> exception (or recurse?). Recursion can get ugly, so perhaps a very 
> explicit error with a reasonable description would be better than 
> letting the DrawImage pipeline fail in various ways.
>
> The helper function can return null if the operation is a NOP (sxy1 ==
> sxy2 - i.e. an empty subimage rectangle), but that case should be 
> handled here and end up with returning ?true? which matches what many 
> of the drawImage() variants return when they detect a trivial NOP case.
>
> The helper function can also return null if the srcWidth/Height are 
> negative which would imply an uninitialized image.  I'm not sure what 
> to do there because normally an uninitialized image is tracked by the 
> DrawImage pipe and the observer is notified as the image data is 
> loaded, but we have no mechanism to do that with an MRI.  This case 
> may require more work, or something explicit in the MRI spec (what 
> happens when someone lazily loads an image with an @x2 variant now?).
>
> It also returns null if the return value from the MRI is a 
> ToolkitImage with an error - but I think that is wrong, it should just 
> return the ToolkitImage and let the caller send it to the DrawImage 
> pipeline to detect the error and respond in the appropriate manner.
> Or perhaps it should cause the drawHiDPIImage() method to return 
> whatever boolean is appropriate for an error to save time, but there 
> is no way to orchestrate that from the helper method since it can only 
> return a (non-MRI) image.  It's probably best to just return the 
> errored TKImage in that case...?
>
> The last way it can return null is if the MRI returned null from its
> getRV() method, but the interface doesn't discuss what that means.  I 
> suppose it might mean that all of the resolution variants are being 
> loaded asynchronously and none of them are available yet, but we are 
> then missing a mechanism to inform the observer of when the variants 
> are loaded.  (Is there a bugid for that?  I seem to recall having a 
> discussion about that missing mechanism in the past...?)
>
> So, I think the change made here is along the right track, but we have 
> a couple more holes to plug before

[OpenJDK 2D-Dev] Review request for JDK - 8146972 : ImageIO reader is not capable of reading JPEGs without JFIF header

[Jayathirth D V]

Hi,

 

This is backport of fix from JDK 9 bug : 
https://bugs.openjdk.java.net/browse/JDK-8041501 to JDK 8u. Changes in JDK9 are 
already checked in.

 

Please review the fix:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8146972 

 

Webrev : http://cr.openjdk.java.net/~jdv/8146972/webrev.00/ 

 

Issue : Pink discoloration when we read JPEG images without JFIF & EXIF header 
and having no subsampling.

 

Root cause : We are overriding JPEG color space set in IJG library at 
imageioJPEG.c without checking component ID's properly when JFIF & EXIF are not 
there. Decision to change color space is solely done consulting sampling 
factors.

 

Solution : Added extra check to verify component ID's also before changing 
color space determined by IJG library when there is no JFIF & EXIF header.

 

Thanks,

Jay

 

[OpenJDK 2D-Dev] Review request for JDK-8144744 : ImageWriter.replacePixels() specification is incorrect regarding null ImageWriteParam

[Jayathirth D V]

Hi,

 

Please review a simple fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8144744 

 

Webrev : http://cr.openjdk.java.net/~jdv/8144744/webrev.00/ 

 

Issue : Specification of ImageWriter.replacePixels() mentions that Illegal 
argument exception will be thrown if argument param is null.

 

Root cause : Implementation of repalcePixels() API is in TIFFImageWriter and we 
actually use default ImageWriteParam if incoming argument param is null and 
don't throw IllegalArgumentException.

 

Solution : Remove the comment in 'Throws' clause of specification which 
mentions that IllegalArgumentException will be thrown if param is null.

 

Thanks,

Jay

 

Re: [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@openjdk.java.net; brian Burkhalter
Subject: Re:  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:  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 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

 

 

Re: [OpenJDK 2D-Dev] [9] RFR: Test closed/javax/print/attribute/Services_getDocFl.java fails with NullpointerException (8040139)

[Jayathirth D V]

Fix looks good to me.

 

From: prasanta sadhukhan 
Sent: Thursday, December 10, 2015 1:40 PM
To: Philip Race; Sergey Bylokhov
Cc: 2d-dev@openjdk.java.net; Rajeev Chamyal
Subject: Re: [OpenJDK 2D-Dev] [9] RFR: Test 
closed/javax/print/attribute/Services_getDocFl.java fails with 
NullpointerException (8040139)

 

Thanks Phil for your approval.
I have opened up the closed test also. Here's the updated webrev:
http://cr.openjdk.java.net/~jdv/prasanta/8040139/webrev.01/

Sergey, would be able to +1 this?

Also, in addition to what Phil mentioned that it is expensive to enumerate for 
available printers in the network, we simply go for this fix to return an array 
with one entry in DocFlavor 
if the printer(s) already registered/added are not available and when user goes 
to print through the dialog, they would see the printer is "stopped" or "unable 
to connect" instead of a NPE.

Regards
Prasanta

On 12/10/2015 12:17 PM, Philip Race wrote:

I had to hunt for this since you did not include the bug number in the subject 
line.
The fix is fine although you really should explain to people why this approach 
is
taken rather than ignoring the printer. i.e it is prohibitively expensive to use
"lpstat -p" when simply enumerating available printers.

And I think the closed test can be opened. I don't see any problem with it and
it was developed internally.

-phil.


On 12/2/15, 2:35 AM, prasanta sadhukhan wrote: 

Hi All,

Please review a fix for jdk9 
Bug: https://bugs.openjdk.java.net/browse/JDK-8040139
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/prasanta/8040139/webrev.00/"http://cr.openjdk.java.net/~jdv/prasanta/8040139/webrev.00/

It seems if a IPP printer is not accessible, 
PrintServiceLookup.lookupPrintServices will return the printer service but 
PrintService.getSupportedDocFlavors() will return null
since for the printer there is no way to answer the questions as to what it 
supports and if after that someone tries to access flavors.length it will cause 
NPE.
However, as per spec
https://docs.oracle.com/javase/8/docs/api/index.html?javax/print/PrintService.html
it clearly says,     

Returns:

Array of supported doc flavors, should have at least one element.


so returning null is a violation of spec. 
I have added a fix to return DocService.SERVICE_FORMATTED flavor for such case.

Could not add a reg testcase as it will need an inaccessible printer.

Regards
Prasanta

 

Re: [OpenJDK 2D-Dev] [9] RFR: Test closed/javax/print/attribute/Services_getDocFl.java fails with NullpointerException (8040139)

[Jayathirth D V]

Since PrintServiceLookup.looupPrintServices is returning a printer service even 
when IPP printer is not accessible. Attributes we want get for the related 
print service should not be null(like print data formats/DocFlavors). Spec also 
mentions the same as pointed by Prasanta(returns at least one Doc Flavor).

 

By doing this we will avoid NPE. Also having user to select another printer 
from dialog(if selected printer is not available) is better than throwing NPE. 
Present changes are fine.

 

Thanks,

Jay

 

From: Jayathirth D V 
Sent: Thursday, December 10, 2015 5:10 PM
To: Prasanta Sadhukhan
Cc: 2d-dev@openjdk.java.net; Rajeev Chamyal; Philip Race; Sergey Bylokhov
Subject: RE: [OpenJDK 2D-Dev] [9] RFR: Test 
closed/javax/print/attribute/Services_getDocFl.java fails with 
NullpointerException (8040139)

 

Fix looks good to me.

 

From: prasanta sadhukhan 
Sent: Thursday, December 10, 2015 1:40 PM
To: Philip Race; Sergey Bylokhov
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net; Rajeev 
Chamyal
Subject: Re: [OpenJDK 2D-Dev] [9] RFR: Test 
closed/javax/print/attribute/Services_getDocFl.java fails with 
NullpointerException (8040139)

 

Thanks Phil for your approval.
I have opened up the closed test also. Here's the updated webrev:
http://cr.openjdk.java.net/~jdv/prasanta/8040139/webrev.01/

Sergey, would be able to +1 this?

Also, in addition to what Phil mentioned that it is expensive to enumerate for 
available printers in the network, we simply go for this fix to return an array 
with one entry in DocFlavor 
if the printer(s) already registered/added are not available and when user goes 
to print through the dialog, they would see the printer is "stopped" or "unable 
to connect" instead of a NPE.

Regards
Prasanta

On 12/10/2015 12:17 PM, Philip Race wrote:

I had to hunt for this since you did not include the bug number in the subject 
line.
The fix is fine although you really should explain to people why this approach 
is
taken rather than ignoring the printer. i.e it is prohibitively expensive to use
"lpstat -p" when simply enumerating available printers.

And I think the closed test can be opened. I don't see any problem with it and
it was developed internally.

-phil.


On 12/2/15, 2:35 AM, prasanta sadhukhan wrote: 

Hi All,

Please review a fix for jdk9 
Bug: https://bugs.openjdk.java.net/browse/JDK-8040139
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/prasanta/8040139/webrev.00/"http://cr.openjdk.java.net/~jdv/prasanta/8040139/webrev.00/

It seems if a IPP printer is not accessible, 
PrintServiceLookup.lookupPrintServices will return the printer service but 
PrintService.getSupportedDocFlavors() will return null
since for the printer there is no way to answer the questions as to what it 
supports and if after that someone tries to access flavors.length it will cause 
NPE.
However, as per spec
https://docs.oracle.com/javase/8/docs/api/index.html?javax/print/PrintService.html
it clearly says,     

Returns:

Array of supported doc flavors, should have at least one element.


so returning null is a violation of spec. 
I have added a fix to return DocService.SERVICE_FORMATTED flavor for such case.

Could not add a reg testcase as it will need an inaccessible printer.

Regards
Prasanta

 

[OpenJDK 2D-Dev] Review request for JDK-6967419 : IndexOutOfBoundsException when drawing PNGs

[Jayathirth D V]

Hi Sergey,

Thanks for your suggestion.
I verified all other image writers with present code(For WBMP after changing 
BufferedImageType to TYPE_BYTE_BINARY) there are no problems in them.

Cache is closed properly and we don't see IndexOutOfBoundsException.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov 
Sent: Wednesday, December 02, 2015 12:43 AM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review request for JDK-6967419 : 
IndexOutOfBoundsException when drawing PNGs

Hi, Jay.
Can you please check other writers and confirm that similar issues are not 
exists there(just try a different writers in the test)? If the problem exists 
it can be fixed as a separate issue, if everything works as expected nothing 
should be changed.

On 17.11.15 14:41, Jayathirth D V wrote:
> Hi Phil,
>
> Thanks for pointing to JDK-8041746 .
>
> _My observations:_
>
> I think with Andrew's test case we are able to identify the problem 
> submitter might have faced in JDK-6967419 . By deliberately throwing 
> exception at count 3L we are trying to reproduce scenario of 
> possible IOException where client is closing the socket(probably 
> because of communication problem between client & server) and IDAT 
> chunk is being written. If we change count to any other number(like 
> 10L) which relates to writing of data apart from IDAT chunk we don't 
> see any issue(no exception and cache is closed properly).
>
> This might explain why submitter is seeing 7 out of 20 fail.
>
> Also by using the test case we are able to see flushedPos going past 
> by
> 4 bytes to Pos when ios.close() is called as mentioned by submitter in 
> JDK-6967419. So catching the IOException and updating 'startPos' 
> value, will not result in IndexOutOfBoundsException and ios.close() 
> will be performed properly.
>
> Please let us know your inputs.
>
> Thanks,
>
> Jay
>
> *From:*Phil Race
> *Sent:* Tuesday, November 17, 2015 3:22 AM
> *To:* Jayathirth D V
> *Cc:* Prasanta Sadhukhan; 2d-dev@openjdk.java.net
> *Subject:* Re: Review request for JDK-6967419 :
> IndexOutOfBoundsException when drawing PNGs
>
> This one reads like it should be obvious but I find it less so ..
> The unsatisfying part is that we do not seem to know what caused the 
> IOException in the customer case.
>
> Andrew came up with a way to reproduce the symptoms but we really 
> don't know what caused the exception in the case of the submitter.
> It does not seem likely he was 'deliberately' throwing an exception to 
> mess up his own application.
>
> I just found this : https://bugs.openjdk.java.net/browse/JDK-8041746
>
> The interesting part is that this bug (the one you are working on) the 
> submitter also wrote that he was using "a ServletOutputStream"
>
> So consequently I wonder if it was something like what is described in
> 8041746 is going on here. It could explain how he sees 7 out of 20 fail.
>
> Please take a look at that one to have a think about it.
> Would your fix help that real world case ?
>
> -phil.
>
> On 11/12/2015 08:11 PM, Jayathirth D V wrote:
>
>     Hi Phil,
>
> I have added public evaluation in bug. Please review.
>
> Thanks,
>
> Jay
>
> *From:*Philip Race
> *Sent:* Friday, November 13, 2015 12:11 AM
> *To:* Jayathirth D V
> *Cc:* Prasanta Sadhukhan; 2d-dev@openjdk.java.net
> <mailto:2d-dev@openjdk.java.net>
> *Subject:* Re: Review request for JDK-6967419 :
> IndexOutOfBoundsException when drawing PNGs
>
> Please add a *public* evaluation to the bug report. I will look at
> it more then ..
>
> -phil.
>
> On 11/6/15, 2:20 AM, Jayathirth D V wrote:
>
> Hi Prasanta,
>
> As discussed, only in case of write_IDAT there is finally block
> which calls ios.finish() which internally calls seek() with
> improper startPos. In other cases we are not trying to access
> improper startPos because there is no call to ios.finish(). We
> can verify this behavior by changing logic where we throw
> IOException in test case.
>
> And I have modified test to not catch IOBE as per your
> suggestion. Please find updated Webrev link:
>
> http://cr.openjdk.java.net/~rchamyal/jay/6967419/webrev.01/
> 
> <http://cr.openjdk.java.net/%7Erchamyal/jay/6967419/webrev.01/>
>
> Thanks,
>
> Jay
>
> *From:*prasanta sadhukhan
> *Sent:* Friday, November 06, 2015 2:45 PM
> *To:* Jayathirth D V; 2d-dev@openjdk.java.net
> <mailto:2d-dev@openjdk.java.net>
> *Cc:* Philip Race
> *Subject

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

[Jayathirth D V]

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/

 

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

Re: [OpenJDK 2D-Dev] Review request for JDK - 8041501 : ImageIO reader is not capable of reading JPEGs without JFIF header

[Jayathirth D V]

Hi Phil,

 

I have replaced already present comment block with proper comments , since it 
was contradicting Metadata spec. And removed extra comments added by me.

 

Please review updated webrev:

 

http://cr.openjdk.java.net/~jdv/8041501/webrev.03/

 

Thanks,

Jay

From: Phil Race 
Sent: Tuesday, November 24, 2015 2:08 AM
To: Jayathirth D V
Cc: Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

Rather than adding a comment block, it seems you need to edit the preceding one.

 * IJG assumes all unidentified 3-channels are YCbCr.
1715  * We assume that only if the second two channels are
1716  * subsampled (either horizontally or vertically).  If not,
1717  * we assume RGB.
 
The fix is invalidating that comment but that is intended since
(a) following the comment is causing the bug
(b) the comment is contradicting the public Metadata spec
 
For the benefit of others, the text in the spec is :

If neither marker segment is present, the following procedure is followed: 
For 3- and 4-channel images, the component ids are consulted.  If these
values are 1-3 for a 3-channel image, then the image is assumed to be YCbCr. 
 
... Otherwise, 3-channel subsampled images are assumed to be YCbCr, 
3-channel non-subsampled images are assumed to be RGB
---
 
So here is my suggested replacement text for the existing comment block :
---
In the absence of certain markers, IJG has interpreted component id's of 
[1,2,3] as meaning YCbCr.
We follow that interpretation, which is additionally described in the Image I/O 
JPEG metadata spec.
If that condition is not met here the next step here is to examine the 
subsampling factors
If any differ we also assume YCbCr, but if all are the same then we assume RGB
This is also described in the Image I/O JPEG metadata spec.

--

-phil.

On 11/23/2015 04:01 AM, Jayathirth D V wrote:

Hi Prasanta,

 

Removed repeated usage of getWidth() and getHeight(). Please review.

 

HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8041501/webrev.02/"http://cr.openjdk.java.net/~jdv/8041501/webrev.02/

 

Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Monday, November 23, 2015 5:15 PM
To: Jayathirth D V
Cc: Philip Race; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

 

On 11/23/2015 5:11 PM, Jayathirth D V wrote:

Hi Prasanta,

 

Thanks for suggestion. I have made related changes and updated the Webrev.

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8041501/webrev.01/"http://cr.openjdk.java.net/~jdv/8041501/webrev.01/

 

Please review.

 

There's no point getting the width & height repeatedly . You can get it once 
and use that info in the loop. It's not going to change in runtime, isn;t it :-)

Regards
Prasanta




Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Monday, November 23, 2015 4:43 PM
To: Jayathirth D V; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Cc: Philip Race
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

Looks ok to me.
But probably you could check for image width  programmatically instead 
of hardcoding to 64 in testcode.

Regards
Prasanta

On 11/23/2015 4:09 PM, Jayathirth D V wrote:

Hello All,

 

Please review following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8041501

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8041501/webrev.00/"http://cr.openjdk.java.net/~jdv/8041501/webrev.00/

 

Issue : Pink discoloration when we read JPEG images without JFIF & EXIF header 
and having no subsampling.

 

Root cause : We are overriding JPEG color space set in IJG library at 
imageioJPEG.c without checking component ID's properly when JFIF & EXIF are not 
there. Decision to change color space is solely done consulting sampling 
factors.

 

Solution : Added extra check to verify component ID's also before changing 
color space determined by IJG library when there is no JFIF & EXIF header.

 

Thanks,

Jay

 

 

 

[OpenJDK 2D-Dev] Review request for JDK-8074967: JPEGImageReader incorrectly identifies YCbCr JPEGs as RGB in standard metadata

[Jayathirth D V]

Hello All,

 

Changed test image used in previous webrev. Because I need image with similar 
properties in another fix(JDK - 8041501) but content should be white.

Changed image and its name in test file no other code changes.

 

Updated Webrev: 

 

http://cr.openjdk.java.net/~rchamyal/jay/8074967/webrev.01/

 

Please review.

 

Thanks,

Jay

 

From: Jayathirth D V 
Sent: Thursday, November 19, 2015 3:19 PM
To: Philip Race; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject:  Review request for JDK-8074967: JPEGImageReader 
incorrectly identifies YCbCr JPEGs as RGB in standard metadata

 

Hi Phil/Prasanta,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8074967

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/8074967/webrev.00/

 

Bug : JPEGImageReader incorrectly identifies YCbCr JPEGs as RGB in standard 
metadata

 

Root cause : In JPEG image if there is no JFIF or EXIF header. We are not 
checking component ID's to determine color space properly. Third channel 
component ID (3) is matching the length of componentSpecs length and we are not 
setting colorspace as YCbCr. It is set to RGB which is wrong.

 

Solution : We should check for id greater than componentSpecs length and not 
greater than equal. But adding a tighter check to match individual component ID 
is better. So made changes to match individual component ID with 1, 2 & 3 to 
set color space as YCbCr.

 

Thanks,

Jay

 

[OpenJDK 2D-Dev] Review request for JDK - 8041501 : ImageIO reader is not capable of reading JPEGs without JFIF header

[Jayathirth D V]

Hello All,

 

Please review following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8041501

 

Webrev : http://cr.openjdk.java.net/~jdv/8041501/webrev.00/

 

Issue : Pink discoloration when we read JPEG images without JFIF & EXIF header 
and having no subsampling.

 

Root cause : We are overriding JPEG color space set in IJG library at 
imageioJPEG.c without checking component ID's properly when JFIF & EXIF are not 
there. Decision to change color space is solely done consulting sampling 
factors.

 

Solution : Added extra check to verify component ID's also before changing 
color space determined by IJG library when there is no JFIF & EXIF header.

 

Thanks,

Jay

Re: [OpenJDK 2D-Dev] Review request for JDK - 8041501 : ImageIO reader is not capable of reading JPEGs without JFIF header

[Jayathirth D V]

Hi Prasanta,

 

Thanks for suggestion. I have made related changes and updated the Webrev.

 

Webrev : http://cr.openjdk.java.net/~jdv/8041501/webrev.01/

 

Please review.

 

Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Monday, November 23, 2015 4:43 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Cc: Philip Race
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

Looks ok to me.
But probably you could check for image width  programmatically instead 
of hardcoding to 64 in testcode.

Regards
Prasanta

On 11/23/2015 4:09 PM, Jayathirth D V wrote:

Hello All,

 

Please review following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8041501

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8041501/webrev.00/"http://cr.openjdk.java.net/~jdv/8041501/webrev.00/

 

Issue : Pink discoloration when we read JPEG images without JFIF & EXIF header 
and having no subsampling.

 

Root cause : We are overriding JPEG color space set in IJG library at 
imageioJPEG.c without checking component ID's properly when JFIF & EXIF are not 
there. Decision to change color space is solely done consulting sampling 
factors.

 

Solution : Added extra check to verify component ID's also before changing 
color space determined by IJG library when there is no JFIF & EXIF header.

 

Thanks,

Jay

 

[OpenJDK 2D-Dev] Review request for JDK - 8041501 : ImageIO reader is not capable of reading JPEGs without JFIF header

[Jayathirth D V]

Hi Prasanta,

 

Removed repeated usage of getWidth() and getHeight(). Please review.

 

http://cr.openjdk.java.net/~jdv/8041501/webrev.02/

 

Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Monday, November 23, 2015 5:15 PM
To: Jayathirth D V
Cc: Philip Race; 2d-dev@openjdk.java.net
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

 

On 11/23/2015 5:11 PM, Jayathirth D V wrote:

Hi Prasanta,

 

Thanks for suggestion. I have made related changes and updated the Webrev.

 

Webrev : http://cr.openjdk.java.net/~jdv/8041501/webrev.01/

 

Please review.

 

There's no point getting the width & height repeatedly . You can get it once 
and use that info in the loop. It's not going to change in runtime, isn;t it :-)

Regards
Prasanta



Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Monday, November 23, 2015 4:43 PM
To: Jayathirth D V; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Cc: Philip Race
Subject: Re:  Review request for JDK - 8041501 : ImageIO reader 
is not capable of reading JPEGs without JFIF header

 

Looks ok to me.
But probably you could check for image width  programmatically instead 
of hardcoding to 64 in testcode.

Regards
Prasanta

On 11/23/2015 4:09 PM, Jayathirth D V wrote:

Hello All,

 

Please review following fix in JDK9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8041501

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8041501/webrev.00/"http://cr.openjdk.java.net/~jdv/8041501/webrev.00/

 

Issue : Pink discoloration when we read JPEG images without JFIF & EXIF header 
and having no subsampling.

 

Root cause : We are overriding JPEG color space set in IJG library at 
imageioJPEG.c without checking component ID's properly when JFIF & EXIF are not 
there. Decision to change color space is solely done consulting sampling 
factors.

 

Solution : Added extra check to verify component ID's also before changing 
color space determined by IJG library when there is no JFIF & EXIF header.

 

Thanks,

Jay

 

 

[OpenJDK 2D-Dev] Review request for JDK-8074967: JPEGImageReader incorrectly identifies YCbCr JPEGs as RGB in standard metadata

[Jayathirth D V]

Hi Phil/Prasanta,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8074967

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/8074967/webrev.00/

 

Bug : JPEGImageReader incorrectly identifies YCbCr JPEGs as RGB in standard 
metadata

 

Root cause : In JPEG image if there is no JFIF or EXIF header. We are not 
checking component ID's to determine color space properly. Third channel 
component ID (3) is matching the length of componentSpecs length and we are not 
setting colorspace as YCbCr. It is set to RGB which is wrong.

 

Solution : We should check for id greater than componentSpecs length and not 
greater than equal. But adding a tighter check to match individual component ID 
is better. So made changes to match individual component ID with 1, 2 & 3 to 
set color space as YCbCr.

 

Thanks,

Jay

 

Re: [OpenJDK 2D-Dev] Review request for JDK-6967419 : IndexOutOfBoundsException when drawing PNGs

[Jayathirth D V]

Hi Phil,

 

Thanks for pointing to JDK-8041746 .

 

My observations:

I think with Andrew's test case we are able to identify the problem submitter 
might have faced in JDK-6967419 . By deliberately throwing exception at count 
3L we are trying to reproduce scenario of possible IOException where client 
is closing the socket(probably because of communication problem between client 
& server) and IDAT chunk is being written. If we change count to any other 
number(like 10L) which relates to writing of data apart from IDAT chunk we 
don't see any issue(no exception and cache is closed properly).

 

This might explain why submitter is seeing 7 out of 20 fail.

 

Also by using the test case we are able to see flushedPos going past by 4 bytes 
to Pos when ios.close() is called as mentioned by submitter in JDK-6967419. So 
catching the IOException and updating 'startPos' value, will not result in 
IndexOutOfBoundsException and ios.close() will be performed properly.

 

Please let us know your inputs.

 

Thanks,

Jay

 

From: Phil Race 
Sent: Tuesday, November 17, 2015 3:22 AM
To: Jayathirth D V
Cc: Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: Review request for JDK-6967419 : IndexOutOfBoundsException when 
drawing PNGs

 

This one reads like it should be obvious but I find it less so ..
The unsatisfying part is that we do not seem to know what caused
the IOException in the customer case.

Andrew came up with a way to reproduce the symptoms but we really
don't know what caused the exception in the case of the submitter.
It does not seem likely he was 'deliberately' throwing an exception to mess up 
his own application.

I just found this : https://bugs.openjdk.java.net/browse/JDK-8041746

The interesting part is that this bug (the one you are working on)
the submitter also wrote that he was using "a ServletOutputStream"

So consequently I wonder if it was something like what is described in
8041746 is going on here. It could explain how he sees 7 out of 20 fail.

Please take a look at that one to have a think about it.
Would your fix help that real world case ?

-phil.

On 11/12/2015 08:11 PM, Jayathirth D V wrote:

Hi Phil,

 

I have added public evaluation in bug. Please review.

 

Thanks,

Jay

 

From: Philip Race 
Sent: Friday, November 13, 2015 12:11 AM
To: Jayathirth D V
Cc: Prasanta Sadhukhan; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: Review request for JDK-6967419 : IndexOutOfBoundsException when 
drawing PNGs

 

Please add a *public* evaluation to the bug report. I will look at it more then 
..

-phil.

On 11/6/15, 2:20 AM, Jayathirth D V wrote: 

Hi Prasanta,

 

As discussed, only in case of write_IDAT there is finally block which calls 
ios.finish() which internally calls seek() with improper startPos. In other 
cases we are not trying to access improper startPos because there is no call to 
ios.finish(). We can verify this behavior by changing logic where we throw 
IOException in test case.

 

And I have modified test to not catch IOBE as per your suggestion. Please find 
updated Webrev link:

 

HYPERLINK 
"http://cr.openjdk.java.net/%7Erchamyal/jay/6967419/webrev.01/"http://cr.openjdk.java.net/~rchamyal/jay/6967419/webrev.01/

 

Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Friday, November 06, 2015 2:45 PM
To: Jayathirth D V; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Cc: Philip Race
Subject: Re: Review request for JDK-6967419 : IndexOutOfBoundsException when 
drawing PNGs

 

Hi Jay,

looks ok but
I guess you need to do the same for finish() method too in similar way you did 
for finishChunk() as finish() is called from write_IHDR, write_CHRM etc and it 
calls flushBefore().
Also, I guess you should not consume IOB Exception and let it be thrown to user 
instead of RuntimeException after catching IOBE.

Regards
Prasanta

On 11/5/2015 5:25 PM, Jayathirth D V wrote:

Hello All,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-6967419

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Erchamyal/jay/6967419/webrev.00/"http://cr.openjdk.java.net/~rchamyal/jay/6967419/webrev.00/

 

Bug : IndexOutOfBoundsException when drawing PNGs

 

Root cause : When user intentionally throws IO Exception while write is 
happening. 
  We call ios.finish() in finally block of write_IDAT() 
which internally goes to finishChunk(). But the startPos of the chunk is still 
pointing to present IDAT chunk but flushedPos(streamPos) is pointing to end of  
IDAT chunk. 
  So in finishChunk(), startPos will be less than 
flushedPos. This is causing IndexOutOfBoundException in stream.seek() and cache 
is not closed.

 

Solution : If IOException is thrown by user, catch the exception while write is 
happening and update startPos to streamPos. So that when seek() happens in 
finishCh

[OpenJDK 2D-Dev] Review request for JDK-6967419 : IndexOutOfBoundsException when drawing PNGs

[Jayathirth D V]

Hi Phil,

 

I have added public evaluation in bug. Please review.

 

Thanks,

Jay

 

From: Philip Race 
Sent: Friday, November 13, 2015 12:11 AM
To: Jayathirth D V
Cc: Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Re: Review request for JDK-6967419 : IndexOutOfBoundsException when 
drawing PNGs

 

Please add a *public* evaluation to the bug report. I will look at it more then 
..

-phil.

On 11/6/15, 2:20 AM, Jayathirth D V wrote: 

Hi Prasanta,

 

As discussed, only in case of write_IDAT there is finally block which calls 
ios.finish() which internally calls seek() with improper startPos. In other 
cases we are not trying to access improper startPos because there is no call to 
ios.finish(). We can verify this behavior by changing logic where we throw 
IOException in test case.

 

And I have modified test to not catch IOBE as per your suggestion. Please find 
updated Webrev link:

 

HYPERLINK 
"http://cr.openjdk.java.net/%7Erchamyal/jay/6967419/webrev.01/"http://cr.openjdk.java.net/~rchamyal/jay/6967419/webrev.01/

 

Thanks,

Jay

 

From: prasanta sadhukhan 
Sent: Friday, November 06, 2015 2:45 PM
To: Jayathirth D V; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Cc: Philip Race
Subject: Re: Review request for JDK-6967419 : IndexOutOfBoundsException when 
drawing PNGs

 

Hi Jay,

looks ok but
I guess you need to do the same for finish() method too in similar way you did 
for finishChunk() as finish() is called from write_IHDR, write_CHRM etc and it 
calls flushBefore().
Also, I guess you should not consume IOB Exception and let it be thrown to user 
instead of RuntimeException after catching IOBE.

Regards
Prasanta

On 11/5/2015 5:25 PM, Jayathirth D V wrote:

Hello All,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-6967419

 

Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Erchamyal/jay/6967419/webrev.00/"http://cr.openjdk.java.net/~rchamyal/jay/6967419/webrev.00/

 

Bug : IndexOutOfBoundsException when drawing PNGs

 

Root cause : When user intentionally throws IO Exception while write is 
happening. 
  We call ios.finish() in finally block of write_IDAT() 
which internally goes to finishChunk(). But the startPos of the chunk is still 
pointing to present IDAT chunk but flushedPos(streamPos) is pointing to end of  
IDAT chunk. 
  So in finishChunk(), startPos will be less than 
flushedPos. This is causing IndexOutOfBoundException in stream.seek() and cache 
is not closed.

 

Solution : If IOException is thrown by user, catch the exception while write is 
happening and update startPos to streamPos. So that when seek() happens in 
finishChunk() we don't see IndexOutOfBoundsException and cache is closed 
properly.

 

Thanks,

Jay

 

 

Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hi Vadim,

I think doing compare and then equals, actually increases the computation we 
are doing to check whether expected value and returned value are same.

New approach of directly using equals to compare between expected and returned 
value is efficient.

I have made changes you mentioned regarding the typo in "spacing".  Please find 
updated Webrev :

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.06/

Please review so that we can push the change.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev 
Sent: Monday, October 19, 2015 4:03 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

I'm sorry, actually the usage of Float.compare was perfectly fine in your case, 
given that you were comparing floats (not Floats).
The thing which caught my eye was the use of Integer boxing as Sergey pointed 
out.
Sorry about the confusion.

Thanks,
Vadim

On 19.10.2015 12:04, Jayathirth D V wrote:
> Hi Vadim,
>
> Thanks for the review.
> I have made suggested changes. Updated Webrev :
> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/
>
> Please review.
>
> Thanks,
> Jay
>
> -Original Message-
> From: Vadim Pakhnushev
> Sent: Friday, October 16, 2015 8:15 PM
> To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip 
> Race
> Subject: Re: [OpenJDK 2D-Dev]  Review request for 
> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>
> Hi Jay,
>
> What's the point of using Float.compare in the test?
> Why not just check if
> (horizontalNodeValue.equals(expectedHorizontalValue)) ?
> Also please capitalize Attr in the declaration of horizontalattr and 
> verticalattr.
>
> Thanks,
> Vadim
>
> On 16.10.2015 17:36, Jayathirth D V wrote:
>> Hello All,
>>
>> Can I get one more review please.
>>
>> Thanks,
>> Jay
>>
>> -Original Message-
>> From: Sergey Bylokhov
>> Sent: Thursday, October 15, 2015 6:05 PM
>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
>> Subject: Re:  Review request for JDK-7182758:
>> BMPMetadata returns invalid PhysicalPixelSpacing
>>
>> The fix looks fine. The test can be improved a little bit to simplify the 
>> int->Integer boxing, but it is not necessary for now.
>>
>> Thanks.
>>
>> On 15.10.15 13:17, Jayathirth D V wrote:
>>> Hi Sergey,
>>>
>>> I thought you suggested to check for tighter "true" condition instead of 
>>> "false" condition.
>>>
>>> I have made changes to map horizontalNodeValue and verticalNodeValue to 
>>> expected values. Please find updated Webrev link:
>>>
>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/
>>>
>>> Please review.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -Original Message-
>>> From: Sergey Bylokhov
>>> Sent: Wednesday, October 14, 2015 7:34 PM
>>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
>>> Subject: Re:  Review request for JDK-7182758:
>>> BMPMetadata returns invalid PhysicalPixelSpacing
>>>
>>> Hi, Jay.
>>>> I suggest to check correct/expected result in the test instead of previous 
>>>> incorrect zero.
>>> Here, I suggested to check that the resulted horizontalNodeValue and 
>>> verticalNodeValue are equal to some expected value. Because if it bigger 
>>> than zero does not mean it is correct(note to use Float.compare(f1, f2) 
>>> instead of "==").
>>>
>>>> Previously I forgot to mention, that it will be good to name the test by 
>>>> some useful name instead of some bug number(see examples in 
>>>> javax/imageio/plugins).
>>>>
>>>> On 13.10.15 11:12, Jayathirth D V wrote:
>>>>> Hello All,
>>>>>
>>>>> Removed Trailing whitespace present in code.
>>>>>
>>>>> Please find updated webrev link:
>>>>>
>>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jay
>>>>>
>>>>> *From:* Jayathirth D V
>>>>> *Sent:* Monday, October 12, 2015 2:15 PM
>>>>> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
>>>>> *Subject:* [OpenJDK 2D-Dev]  Review request for
>>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>

Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev 
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and 
verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:
> Hello All,
>
> Can I get one more review please.
>
> Thanks,
> Jay
>
> -Original Message-
> From: Sergey Bylokhov
> Sent: Thursday, October 15, 2015 6:05 PM
> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
> Subject: Re:  Review request for JDK-7182758: 
> BMPMetadata returns invalid PhysicalPixelSpacing
>
> The fix looks fine. The test can be improved a little bit to simplify the 
> int->Integer boxing, but it is not necessary for now.
>
> Thanks.
>
> On 15.10.15 13:17, Jayathirth D V wrote:
>> Hi Sergey,
>>
>> I thought you suggested to check for tighter "true" condition instead of 
>> "false" condition.
>>
>> I have made changes to map horizontalNodeValue and verticalNodeValue to 
>> expected values. Please find updated Webrev link:
>>
>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/
>>
>> Please review.
>>
>> Thanks,
>> Jay
>>
>> -Original Message-
>> From: Sergey Bylokhov
>> Sent: Wednesday, October 14, 2015 7:34 PM
>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
>> Subject: Re:  Review request for JDK-7182758:
>> BMPMetadata returns invalid PhysicalPixelSpacing
>>
>> Hi, Jay.
>>> I suggest to check correct/expected result in the test instead of previous 
>>> incorrect zero.
>> Here, I suggested to check that the resulted horizontalNodeValue and 
>> verticalNodeValue are equal to some expected value. Because if it bigger 
>> than zero does not mean it is correct(note to use Float.compare(f1, f2) 
>> instead of "==").
>>
>>> Previously I forgot to mention, that it will be good to name the test by 
>>> some useful name instead of some bug number(see examples in 
>>> javax/imageio/plugins).
>>>
>>> On 13.10.15 11:12, Jayathirth D V wrote:
>>>> Hello All,
>>>>
>>>> Removed Trailing whitespace present in code.
>>>>
>>>> Please find updated webrev link:
>>>>
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>> *From:* Jayathirth D V
>>>> *Sent:* Monday, October 12, 2015 2:15 PM
>>>> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
>>>> *Subject:* [OpenJDK 2D-Dev]  Review request for
>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Hello All,
>>>>
>>>> Made small change on how we need to represent floating point 
>>>> constant in
>>>> Java(1000.0 -> 1000.0F).
>>>>
>>>> Please find updated Webrev link:
>>>>
>>>> Webrev : 
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/
>>>>
>>>> Please review.
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>> *From:* Jayathirth D V
>>>> *Sent:* Thursday, October 08, 2015 2:20 PM
>>>> *To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>; 
>>>> Philip Race; Sergey Bylokhov
>>>> *Subject:* [OpenJDK 2D-Dev]  Review request for
>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Hello All,
>>>>
>>>> Please review following fix in jdk9:
>>>>
>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758
>>>>
>>>> Webrev : 
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/
>>>>
>>>> Bug : BMPMetadata returns invalid PhysicalPixelSpacing
>>>>
>>>> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more 
>>>> than value 1 in BMP header. Horizontal & Vertical Physical pixel 
>>>> spacing were returned as zero.
>>>>
>>>>   In getStandardDimensionNode() method 
>>>> of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter.
>>>> When
>>>>
>>>>   XPixelsPerMter/ YPixelsPerMter is 
>>>> more than 1. Resulted value is stored without decimal part, which resulted 
>>>> in zero.
>>>>
>>>> Solution : Made changes to how Horizontal & Vertical Physical pixel 
>>>> spacing is calculated so that decimal value is not truncated.
>>>>
>>>> Thanks,
>>>>
>>>> Jay
>>>>
>>>
>>> --
>>> Best regards, Sergey.
>>>
>>
>> --
>> Best regards, Sergey.
>>
>
> --
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hi Vadim,

Thanks for throwing light on performance aspect of Boxing & Unboxing in Java.

I have made changes, so that we use Float.compare and then use equality 
operator to determine whether expected & returned values are same.

Please find updated Webrev:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.07/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev 
Sent: Monday, October 19, 2015 4:50 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Jay,
What I mean is that you can either declare two floats (lowercase) and then you 
need to do if (Float.compare(f1, f2) == 0) Or you declare a Float f1 and then 
you can write if (f1.equals(f2)) In the first case there isn't any boxing, 
while in the second case second float will be boxed (and unboxed in the equals 
method).
So technically Float.compare(f1, f2) == 0 is the most efficient way to compare 
two primitive floats, especially given that Float.parseFloat returns primitive 
float.
In this particular case simple == would be sufficient though, since one of the 
floats is computed at compile time and you know that you won't be comparing 
NaN's and expecting that they are equal...

I'm OK with both approaches, but would prefer primitive types and 
(Float.compare(f1, f2) == 0).

Thanks,
Vadim

On 19.10.2015 14:04, Jayathirth D V wrote:
> Hi Vadim,
>
> I think doing compare and then equals, actually increases the computation we 
> are doing to check whether expected value and returned value are same.
>
> New approach of directly using equals to compare between expected and 
> returned value is efficient.
>
> I have made changes you mentioned regarding the typo in "spacing".  Please 
> find updated Webrev :
>
> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.06/
>
> Please review so that we can push the change.
>
> Thanks,
> Jay
>
> -Original Message-----
> From: Vadim Pakhnushev
> Sent: Monday, October 19, 2015 4:03 PM
> To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip 
> Race
> Subject: Re: [OpenJDK 2D-Dev]  Review request for 
> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>
> Hi Jay,
>
> I'm sorry, actually the usage of Float.compare was perfectly fine in your 
> case, given that you were comparing floats (not Floats).
> The thing which caught my eye was the use of Integer boxing as Sergey pointed 
> out.
> Sorry about the confusion.
>
> Thanks,
> Vadim
>
> On 19.10.2015 12:04, Jayathirth D V wrote:
>> Hi Vadim,
>>
>> Thanks for the review.
>> I have made suggested changes. Updated Webrev :
>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/
>>
>> Please review.
>>
>> Thanks,
>> Jay
>>
>> -Original Message-
>> From: Vadim Pakhnushev
>> Sent: Friday, October 16, 2015 8:15 PM
>> To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip 
>> Race
>> Subject: Re: [OpenJDK 2D-Dev]  Review request for
>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>
>> Hi Jay,
>>
>> What's the point of using Float.compare in the test?
>> Why not just check if
>> (horizontalNodeValue.equals(expectedHorizontalValue)) ?
>> Also please capitalize Attr in the declaration of horizontalattr and 
>> verticalattr.
>>
>> Thanks,
>> Vadim
>>
>> On 16.10.2015 17:36, Jayathirth D V wrote:
>>> Hello All,
>>>
>>> Can I get one more review please.
>>>
>>> Thanks,
>>> Jay
>>>
>>> -Original Message-
>>> From: Sergey Bylokhov
>>> Sent: Thursday, October 15, 2015 6:05 PM
>>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
>>> Subject: Re:  Review request for JDK-7182758:
>>> BMPMetadata returns invalid PhysicalPixelSpacing
>>>
>>> The fix looks fine. The test can be improved a little bit to simplify the 
>>> int->Integer boxing, but it is not necessary for now.
>>>
>>> Thanks.
>>>
>>> On 15.10.15 13:17, Jayathirth D V wrote:
>>>> Hi Sergey,
>>>>
>>>> I thought you suggested to check for tighter "true" condition instead of 
>>>> "false" condition.
>>>>
>>>> I have made changes to map horizontalNodeValue and verticalNodeValue to 
>>>> expected values. Please find updated Webrev link:
>>>>
>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/
>>>>
>>>> Please revie

Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hello All,

Can I get one more review please.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov 
Sent: Thursday, October 15, 2015 6:05 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758: BMPMetadata 
returns invalid PhysicalPixelSpacing

The fix looks fine. The test can be improved a little bit to simplify the 
int->Integer boxing, but it is not necessary for now.

Thanks.

On 15.10.15 13:17, Jayathirth D V wrote:
> Hi Sergey,
>
> I thought you suggested to check for tighter "true" condition instead of 
> "false" condition.
>
> I have made changes to map horizontalNodeValue and verticalNodeValue to 
> expected values. Please find updated Webrev link:
>
> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/
>
> Please review.
>
> Thanks,
> Jay
>
> -Original Message-
> From: Sergey Bylokhov
> Sent: Wednesday, October 14, 2015 7:34 PM
> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
> Subject: Re:  Review request for JDK-7182758: 
> BMPMetadata returns invalid PhysicalPixelSpacing
>
> Hi, Jay.
>> I suggest to check correct/expected result in the test instead of previous 
>> incorrect zero.
>
> Here, I suggested to check that the resulted horizontalNodeValue and 
> verticalNodeValue are equal to some expected value. Because if it bigger than 
> zero does not mean it is correct(note to use Float.compare(f1, f2) instead of 
> "==").
>
>>
>> Previously I forgot to mention, that it will be good to name the test by 
>> some useful name instead of some bug number(see examples in 
>> javax/imageio/plugins).
>>
>> On 13.10.15 11:12, Jayathirth D V wrote:
>>> Hello All,
>>>
>>> Removed Trailing whitespace present in code.
>>>
>>> Please find updated webrev link:
>>>
>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>> *From:* Jayathirth D V
>>> *Sent:* Monday, October 12, 2015 2:15 PM
>>> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
>>> *Subject:* [OpenJDK 2D-Dev]  Review request for
>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>
>>> Hello All,
>>>
>>> Made small change on how we need to represent floating point 
>>> constant in
>>> Java(1000.0 -> 1000.0F).
>>>
>>> Please find updated Webrev link:
>>>
>>> Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/
>>>
>>> Please review.
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>> *From:* Jayathirth D V
>>> *Sent:* Thursday, October 08, 2015 2:20 PM
>>> *To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>; 
>>> Philip Race; Sergey Bylokhov
>>> *Subject:* [OpenJDK 2D-Dev]  Review request for
>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>>>
>>> Hello All,
>>>
>>> Please review following fix in jdk9:
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758
>>>
>>> Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/
>>>
>>> Bug : BMPMetadata returns invalid PhysicalPixelSpacing
>>>
>>> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more than 
>>> value 1 in BMP header. Horizontal & Vertical Physical pixel spacing 
>>> were returned as zero.
>>>
>>>  In getStandardDimensionNode() method of 
>>> BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter.
>>> When
>>>
>>>  XPixelsPerMter/ YPixelsPerMter is more 
>>> than 1. Resulted value is stored without decimal part, which resulted in 
>>> zero.
>>>
>>> Solution : Made changes to how Horizontal & Vertical Physical pixel 
>>> spacing is calculated so that decimal value is not truncated.
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>
>>
>> --
>> Best regards, Sergey.
>>
>
>
> --
> Best regards, Sergey.
>


--
Best regards, Sergey.

[OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hi Sergey,

I have made suggested changes. Please find updated Webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.03/

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov 
Sent: Tuesday, October 13, 2015 9:06 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758: BMPMetadata 
returns invalid PhysicalPixelSpacing

Hi, Jay.
I suggest to check correct/expected result in the test instead of previous 
incorrect zero.

Previously I forgot to mention, that it will be good to name the test by some 
useful name instead of some bug number(see examples in javax/imageio/plugins).

On 13.10.15 11:12, Jayathirth D V wrote:
> Hello All,
>
> Removed Trailing whitespace present in code.
>
> Please find updated webrev link:
>
> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/
>
> Thanks,
>
> Jay
>
> *From:* Jayathirth D V
> *Sent:* Monday, October 12, 2015 2:15 PM
> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
> *Subject:* [OpenJDK 2D-Dev]  Review request for
> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>
> Hello All,
>
> Made small change on how we need to represent floating point constant 
> in
> Java(1000.0 -> 1000.0F).
>
> Please find updated Webrev link:
>
> Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/
>
> Please review.
>
> Thanks,
>
> Jay
>
> *From:* Jayathirth D V
> *Sent:* Thursday, October 08, 2015 2:20 PM
> *To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>; Philip 
> Race; Sergey Bylokhov
> *Subject:* [OpenJDK 2D-Dev]  Review request for
> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
>
> Hello All,
>
> Please review following fix in jdk9:
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758
>
> Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/
>
> Bug : BMPMetadata returns invalid PhysicalPixelSpacing
>
> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more than 
> value 1 in BMP header. Horizontal & Vertical Physical pixel spacing 
> were returned as zero.
>
>In getStandardDimensionNode() method of 
> BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter. 
> When
>
>XPixelsPerMter/ YPixelsPerMter is more than 
> 1. Resulted value is stored without decimal part, which resulted in zero.
>
> Solution : Made changes to how Horizontal & Vertical Physical pixel 
> spacing is calculated so that decimal value is not truncated.
>
> Thanks,
>
> Jay
>


--
Best regards, Sergey.

[OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hello All,

 

Removed Trailing whitespace present in code.

 

Please find updated webrev link:

 

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/

 

Thanks,

Jay

 

From: Jayathirth D V 
Sent: Monday, October 12, 2015 2:15 PM
To: 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
Subject: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

 

Hello All,

 

Made small change on how we need to represent floating point constant in 
Java(1000.0 -> 1000.0F).

 

Please find updated Webrev link:

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/

 

Please review.

 

Thanks,

Jay

 

 

From: Jayathirth D V 
Sent: Thursday, October 08, 2015 2:20 PM
To: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net; Philip 
Race; Sergey Bylokhov
Subject: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

 

Hello All,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-7182758

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/

 

Bug : BMPMetadata returns invalid PhysicalPixelSpacing

 

Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more than value 1 in 
BMP header. Horizontal & Vertical Physical pixel spacing were returned as zero.

  In getStandardDimensionNode() method of 
BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter. When

  XPixelsPerMter/ YPixelsPerMter is more than 1. 
Resulted value is stored without decimal part, which resulted in zero.

 

Solution : Made changes to how Horizontal & Vertical Physical pixel spacing is 
calculated so that decimal value is not truncated.

 

Thanks,

Jay

[OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hello All,

 

Made small change on how we need to represent floating point constant in 
Java(1000.0 -> 1000.0F).

 

Please find updated Webrev link:

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/

 

Please review.

 

Thanks,

Jay

 

 

From: Jayathirth D V 
Sent: Thursday, October 08, 2015 2:20 PM
To: 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
Subject: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

 

Hello All,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-7182758

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/

 

Bug : BMPMetadata returns invalid PhysicalPixelSpacing

 

Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more than value 1 in 
BMP header. Horizontal & Vertical Physical pixel spacing were returned as zero.

  In getStandardDimensionNode() method of 
BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter. When

  XPixelsPerMter/ YPixelsPerMter is more than 1. 
Resulted value is stored without decimal part, which resulted in zero.

 

Solution : Made changes to how Horizontal & Vertical Physical pixel spacing is 
calculated so that decimal value is not truncated.

 

Thanks,

Jay

[OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

[Jayathirth D V]

Hello All,

 

Please review following fix in jdk9:

 

Bug : https://bugs.openjdk.java.net/browse/JDK-7182758

 

Webrev : http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/

 

Bug : BMPMetadata returns invalid PhysicalPixelSpacing

 

Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more than value 1 in 
BMP header. Horizontal & Vertical Physical pixel spacing were returned as zero.

  In getStandardDimensionNode() method of 
BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter. When

  XPixelsPerMter/ YPixelsPerMter is more than 1. 
Resulted value is stored without decimal part, which resulted in zero.

 

Solution : Made changes to how Horizontal & Vertical Physical pixel spacing is 
calculated so that decimal value is not truncated.

 

Thanks,

Jay

Re: [OpenJDK 2D-Dev] Review request for JDK-8066904: NullPointerExcpetion when calling ImageIO.read() with corrupt BMP

[Jayathirth D v]

Hello All,

Gentle Reminder. Please review the changes.

Thanks,
Jay

On 10/6/2015 12:57 PM, Jayathirth D v wrote:

Hello All,

We noticed that jtreg tag "@run" was not matching main class name for 
test file.

So made relevant change only in comment's section of test file.

Please find updated Webrev link  :

http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.01/

Thanks,
Jay



On 10/6/2015 9:51 AM, Jayathirth D v wrote:

Hi Sergey,

Thanks for review.

Hello All,

I need one more review for this patch. Please review.

Thanks,
Jay

On 10/5/2015 7:01 PM, Sergey Bylokhov wrote:

Hi, Jay.
The fix looks fine to me.

On 05.10.15 13:17, Jayathirth D v wrote:

Hello All,

_Please review following fix in jdk9:_

_Bug:_ https://bugs.openjdk.java.net/browse/JDK-8066904/

_Webrev:_
<http://cr.openjdk.java.net/%7Erchamyal/jay/8066904/webrev.00/>http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.00/ 



_Bug:_ NullPointerException when calling ImageIO.read(InputStream) 
with

corrupt BMP

_Root cause:_ Bits per pixel is zero in BMP header and because of 
which

sampleModel & colorModel are set to null in readHeader() of
BMPImageReader.java. This is causing "bdata" to be null in
read(imageIndex, ImageReadParam) which is referenced later.

_Solution:_ Bits per pixel cant be zero for BI_RGB compression type(no
compression). Cases where Bits per pixel is not supported needs to be
handled properly. So caught exceptions at relevant places.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review request for JDK-8066904: NullPointerExcpetion when calling ImageIO.read() with corrupt BMP

[Jayathirth D v]

Hello All,

We noticed that jtreg tag "@run" was not matching main class name for 
test file.

So made relevant change only in comment's section of test file.

Please find updated Webrev link  :

http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.01/

Thanks,
Jay



On 10/6/2015 9:51 AM, Jayathirth D v wrote:

Hi Sergey,

Thanks for review.

Hello All,

I need one more review for this patch. Please review.

Thanks,
Jay

On 10/5/2015 7:01 PM, Sergey Bylokhov wrote:

Hi, Jay.
The fix looks fine to me.

On 05.10.15 13:17, Jayathirth D v wrote:

Hello All,

_Please review following fix in jdk9:_

_Bug:_ https://bugs.openjdk.java.net/browse/JDK-8066904/

_Webrev:_
<http://cr.openjdk.java.net/%7Erchamyal/jay/8066904/webrev.00/>http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.00/ 



_Bug:_ NullPointerException when calling ImageIO.read(InputStream) with
corrupt BMP

_Root cause:_ Bits per pixel is zero in BMP header and because of which
sampleModel & colorModel are set to null in readHeader() of
BMPImageReader.java. This is causing "bdata" to be null in
read(imageIndex, ImageReadParam) which is referenced later.

_Solution:_ Bits per pixel cant be zero for BI_RGB compression type(no
compression). Cases where Bits per pixel is not supported needs to be
handled properly. So caught exceptions at relevant places.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] Review request for JDK-8066904: NullPointerExcpetion when calling ImageIO.read() with corrupt BMP

[Jayathirth D v]

Hi Sergey,

Thanks for review.

Hello All,

I need one more review for this patch. Please review.

Thanks,
Jay

On 10/5/2015 7:01 PM, Sergey Bylokhov wrote:

Hi, Jay.
The fix looks fine to me.

On 05.10.15 13:17, Jayathirth D v wrote:

Hello All,

_Please review following fix in jdk9:_

_Bug:_ https://bugs.openjdk.java.net/browse/JDK-8066904/

_Webrev:_
<http://cr.openjdk.java.net/%7Erchamyal/jay/8066904/webrev.00/>http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.00/ 



_Bug:_ NullPointerException when calling ImageIO.read(InputStream) with
corrupt BMP

_Root cause:_ Bits per pixel is zero in BMP header and because of which
sampleModel & colorModel are set to null in readHeader() of
BMPImageReader.java. This is causing "bdata" to be null in
read(imageIndex, ImageReadParam) which is referenced later.

_Solution:_ Bits per pixel cant be zero for BI_RGB compression type(no
compression). Cases where Bits per pixel is not supported needs to be
handled properly. So caught exceptions at relevant places.

Thanks,
Jay

[OpenJDK 2D-Dev] Review request for JDK-8066904: NullPointerExcpetion when calling ImageIO.read() with corrupt BMP

[Jayathirth D v]

Hello All,

_Please review following fix in jdk9:_

_Bug:_ https://bugs.openjdk.java.net/browse/JDK-8066904/

_Webrev:_ http://cr.openjdk.java.net/~rchamyal/jay/8066904/webrev.00/ 



_Bug:_ NullPointerException when calling ImageIO.read(InputStream) with 
corrupt BMP


_Root cause:_ Bits per pixel is zero in BMP header and because of which 
sampleModel & colorModel are set to null in readHeader() of 
BMPImageReader.java. This is causing "bdata" to be null in 
read(imageIndex, ImageReadParam) which is referenced later.


_Solution:_ Bits per pixel cant be zero for BI_RGB compression type(no 
compression). Cases where Bits per pixel is not supported needs to be 
handled properly. So caught exceptions at relevant places.


Thanks,
Jay