Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
+1 On 20.10.2015 8:31, Jayathirth D V wrote: 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 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 Traili
Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
Looks fine. On 20.10.15 11:26, Jayathirth D V wrote: Hello All, I need one more review. Please review. Thanks, Jay -Original Message- From: Vadim Pakhnushev Sent: Tuesday, October 20, 2015 11:44 AM 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 +1 On 20.10.2015 8:31, Jayathirth D V wrote: 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 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 vert
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 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
Re: [OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
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
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 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
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
Looks good,
Please fix the typo before the push here ("spcaing"):
throw new RuntimeException("Invalid pixel spcaing");
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
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
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
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
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
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.
Re: [OpenJDK 2D-Dev] 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.
[OpenJDK 2D-Dev] Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing
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
Re: [OpenJDK 2D-Dev] 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
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
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
