'Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid Ph'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid Ph
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2015-10-20 14:52:09
Message-ID: 56265519.6000600 () oracle ! com
[Download RAW message or body]

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] <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] <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] <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] <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: <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: <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] <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] <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.
> 


-- 
Best regards, Sergey.


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic