Re: [OpenJDK 2D-Dev] [12] Review Request: 8214461 Some unused classes may be removed
Looks good to me. Krishna > On 01-Dec-2018, at 7:33 AM, Sergey Bylokhov > wrote: > > Hello. > Please review the fix for jdk 12. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8214461 > Webrev: http://cr.openjdk.java.net/~serb/8214461/webrev.00 > > We have a few internal classes/interfaces which currently unused and may be > removed: > - sun.awt.image.BadDepthException.java: looks like it was never used > - sun.awt.TracedEventQueue.java: was used for logging > - sun.awt.Graphics2Delegate.java: currently unused > > > -- > Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt
Hi Magnus, Thanks for taking a look. I was wanting to change the SAFE_ALLOC definition, but since that file is in java.base, I was not sure of changing it. Krishna From: Magnus Ihse Bursie Sent: Tuesday, November 27, 2018 6:52 PM To: Krishna Addepalli Cc: Philip Race ; awt-...@openjdk.java.net; 2d-dev <2d-dev@openjdk.java.net>; build-dev Subject: Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt I normally do not comment on component source code changes, but I glanced through this and noticed that a lot of size_t values are casted to int, in situations where a size_t is expected, like SAFE_ALLOC or so. Perhaps it would be better to change the argument to those functions, rather than to cast a lot of size_t expressions to int. As a rule of thumb, any expression that measure an amount of memory should be of type size_t, rather than int. /Magnus 27 nov. 2018 kl. 12:14 skrev Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>: Hi Phil, To reduce the scope, I have created a new webrev, which addresses only warnings on Linux platform. Warnings for other platforms will be addressed in separate bugs. Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev02/ For your reference, I’m attaching the warning log generated by the compiler for each warning type. Hope this helps in the review. I ran the all the jtreg tests, but I’m not sure if the changes have caused any problems. I checked with Ajit (who tried to address this issue before), and ran SwingSet2 with GTK2 and GTK3 and did not find any crashes. Thanks, Krishna From: Krishna Addepalli Sent: Tuesday, October 2, 2018 8:53 PM To: Philip Race mailto:philip.r...@oracle.com"philip.r...@oracle.com> Cc: HYPERLINK "mailto:awt-...@openjdk.java.net"awt-...@openjdk.java.net; 2d-dev mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>; build-dev mailto:build-...@openjdk.java.net"build-...@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt Yes, that is right. I have compiled it Mac, Linux and Windows locally. I tried submitting a Mach5 job, but was unable to as it was down. Will try it again. Thanks Krishna On 02-Oct-2018, at 3:39 AM, Philip Race mailto:philip.r...@oracle.com"philip.r...@oracle.com> wrote: I suspect I understand this one now .. the array is stack allocated so we don't want NULL but the compiler probably complained about possible uninitialised use of the values ? -phil. On 10/1/18, 9:38 AM, Philip Race wrote: You really do need to explain *each* of the changes better. This one .. why not NULL ? HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html -phil On 10/1/18, 9:19 AM, Philip Race wrote: Hi, 1) Has this been built on all platforms ? 2) I can't find the list of warnings that you are seeing and fixing and they are all over the place. So adding 2d-dev and build-dev. For each of these changes, please show what was the warning that you received from the compiler This might sound like a lot of work, but it won't be disproportionate and I've made the same request for similar reviews and without it, it is hard to review the changes. For example (and I do mean just example) HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html why would that not be #ifdef instead ? 3) Testing .. did you run at least all our jtreg tests to make sure you didn't break some behaviour .. -phil. On 9/29/18, 8:18 PM, Krishna Addepalli wrote: Hi All, Please review a fix for JDK-8074824: https://bugs.openjdk.java.net/browse/JDK-8074824 Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/ Most of the warnings have been fixed for Linux, Mac and Windows. Thanks, Krishna
Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt
Hi Phil,
To reduce the scope, I have created a new webrev, which addresses only warnings
on Linux platform.
Warnings for other platforms will be addressed in separate bugs.
Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev02/
For your reference, I'm attaching the warning log generated by the compiler for
each warning type. Hope this helps in the review.
I ran the all the jtreg tests, but I'm not sure if the changes have caused any
problems.
I checked with Ajit (who tried to address this issue before), and ran SwingSet2
with GTK2 and GTK3 and did not find any crashes.
Thanks,
Krishna
From: Krishna Addepalli
Sent: Tuesday, October 2, 2018 8:53 PM
To: Philip Race
Cc: awt-...@openjdk.java.net; 2d-dev <2d-dev@openjdk.java.net>; build-dev
Subject: Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve
disabled warnings for libawt_xawt
Yes, that is right.
I have compiled it Mac, Linux and Windows locally. I tried submitting a Mach5
job, but was unable to as it was down. Will try it again.
Thanks
Krishna
On 02-Oct-2018, at 3:39 AM, Philip Race mailto:philip.r...@oracle.com"philip.r...@oracle.com> wrote:
I suspect I understand this one now .. the array is stack allocated so we don't
want NULL
but the compiler probably complained about possible uninitialised use of the
values ?
-phil.
On 10/1/18, 9:38 AM, Philip Race wrote:
You really do need to explain *each* of the changes better.
This one .. why not NULL ?
HYPERLINK
"http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html
-phil
On 10/1/18, 9:19 AM, Philip Race wrote:
Hi,
1) Has this been built on all platforms ?
2) I can't find the list of warnings that you are seeing and fixing and they
are all over the place.
So adding 2d-dev and build-dev.
For each of these changes, please show what was the warning that you received
from the compiler
This might sound like a lot of work, but it won't be disproportionate and I've
made the same
request for similar reviews and without it, it is hard to review the changes.
For example (and I do mean just example)
HYPERLINK
"http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html
why would that not be #ifdef instead ?
3) Testing .. did you run at least all our jtreg tests to make sure you didn't
break
some behaviour ..
-phil.
On 9/29/18, 8:18 PM, Krishna Addepalli wrote:
Hi All,
Please review a fix for JDK-8074824:
https://bugs.openjdk.java.net/browse/JDK-8074824
Webrev: HYPERLINK
"http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/"http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/
Most of the warnings have been fixed for Linux, Mac and Windows.
Thanks,
Krishna
Warning: type-limits
Files Affected:
1. In file included from
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/xawt/XlibWrapper.c:31:0:
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/xawt/XlibWrapper.c:
In function ‘Java_sun_awt_X11_XlibWrapper_SetBitmapShape’:
/home/krishna/jdklatest/open/src/java.base/share/native/libjava/sizecalc.h:47:32:
error: comparison of unsigned expression >= 0 is always true
[-Werror=type-limits]
#define IS_SAFE_SIZE_T(x) ((x) >= 0 && (unsigned long long)(x) <= SIZE_MAX)
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/xawt/XlibWrapper.c:2301:23:
note: in expansion of macro ‘SAFE_SIZE_ARRAY_ALLOC’
2. In file included from
/home/krishna/jdklatest/open/src/java.desktop/unix/native/common/awt/fontpath.c:44:0:
/home/krishna/jdklatest/open/src/java.desktop/unix/native/common/awt/fontpath.c:
In function ‘AddFontsToX11FontPath’:
/home/krishna/jdklatest/open/src/java.base/share/native/libjava/sizecalc.h:47:32:
error: comparison of unsigned expression >= 0 is always true
[-Werror=type-limits]
#define IS_SAFE_SIZE_T(x) ((x) >= 0 && (unsigned long long)(x) <= SIZE_MAX)
/home/krishna/jdklatest/open/src/java.desktop/unix/native/common/awt/fontpath.c:289:19:
note: in expansion of macro ‘SAFE_SIZE_ARRAY_ALLOC’
3.
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:
In function ‘recode_color’:
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:2021:16:
error: comparison is always false due to limited range of data type
[-Werror=type-limits]
if (result > 65535) {
4.
/home/krishna/jdklatest/open/src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:593:34:
note: i
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException
Thanks for the clarification Jay.
The changes look fine.
Krishna
-Original Message-
From: Jayathirth D V
Sent: Tuesday, November 20, 2018 3:37 PM
To: Krishna Addepalli ; 2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Hi Krishna,
Thanks for the review.
I have added detail comment mentioning why we don’t read CRC data for IEND
chunk.
We can add debug/warning info mentioning that CRC data is not present/ corrupt,
but after IEND chunk type there can be any length of stream data. In the stream
present for this bug we have 3 byte data and we are throwing IIOException. If
some image has stream data of more than 4 byte after IEND chunk type, in that
case also we would assume the data present as CRC for IEND chunk and try to
validate it. Basically there is no way we can verify that stream data present
after IEND chunk type is CRC for IEND chunk. So it's better if stop reading
metadata as soon as we hit IEND chunk type, reading more metadata and trying to
validate it is not needed.
Also as I have added in JBS many other decoders(Ubuntu GIMP, IrfanView, Windows
paint, Mac Preview, Ubuntu Image viewer) also work this way and they are not
worried about CRC for IEND chunk.
Thanks,
Jay
-Original Message-
From: Krishna Addepalli
Sent: Tuesday, November 20, 2018 2:32 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Hi Jay,
While the exception is suppressed with your fix, I think it makes sense to add
some logging of this fact, so that it could aid in debugging.
Thanks,
Krishna
-Original Message-
From: Jayathirth D V
Sent: Friday, November 16, 2018 12:30 PM
To: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Thanks Sergey for the approval.
Need one more review.
Please review the webrev :
http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
Regards,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Thursday, November 15, 2018 10:58 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Looks fine.
On 14/11/2018 04:35, Jayathirth D V wrote:
> Hi Sergey,
>
> Thanks for the review.
>
> I have updated the code to not move the changes out of switch. Apart from
> that I have refined comments to make it clear why we are not reading CRC for
> IEND chunk. Also chunkCRC value needs to be initialized because of this
> update, initialized value of chunkCRC will not be used anywhere in the logic.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Sergey Bylokhov
> Sent: Wednesday, November 14, 2018 4:31 AM
> To: Jayathirth D V; 2d-dev@openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
> corrupt CRC for IEND chunk throws IIOException
>
> Hi, Jay.
>
> Probably it will be better to not to change the code inside switch, and only
> add the check below around of reading CRC:
> "if (chunkType != IEND_TYPE) {"
>
> The current fix may throw "Invalid chunk length" when seek/flush the data for
> IEND_TYPE, which is not correct.
>
>
> On 12/11/2018 20:22, Jayathirth D V wrote:
>> Hello All,
>>
>> Gentle reminder for review.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Jayathirth Rao
>> *Sent:* Tuesday, October 23, 2018 7:09 PM
>> *To:* 2d-dev@openjdk.java.net
>> *Subject:* [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
>> corrupt CRC for IEND chunk throws IIOException
>>
>> Hello All,
>>
>> Please review the following fix in JDK12:
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8211422
>>
>> Webrev: http://cr.openjdk.java.net/~jdv/8211422/webrev.00/
>>
>> Issue : When we try to read PNG image with corrupt/no 4 byte CRC data for
>> IEND chunk we throw IIOException. We see this issue only after JDK-8164971
>> <https://bugs.openjdk.java.net/browse/JDK-8164971>.
>>
>> Root cause : In JDK-8164971
>> <https://bugs.openjdk.java.net/browse/JDK-8164971> fix we made changes to
>> continue reading metadata until we reach IEND chunk. Before JDK-8164971
>> <https://bugs.openjdk.java.net/browse/JDK-8164971> change we used to stop
>> reading metadata as soon as we hit first IDAT chunk. According to PNG spec
>> there can be more than one IDAT chunk/ more headers after IDAT chunk. So we
>> ne
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt CRC for IEND chunk throws IIOException
Hi Jay,
While the exception is suppressed with your fix, I think it makes sense to add
some logging of this fact, so that it could aid in debugging.
Thanks,
Krishna
-Original Message-
From: Jayathirth D V
Sent: Friday, November 16, 2018 12:30 PM
To: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Thanks Sergey for the approval.
Need one more review.
Please review the webrev :
http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
Regards,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Thursday, November 15, 2018 10:58 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with corrupt
CRC for IEND chunk throws IIOException
Looks fine.
On 14/11/2018 04:35, Jayathirth D V wrote:
> Hi Sergey,
>
> Thanks for the review.
>
> I have updated the code to not move the changes out of switch. Apart from
> that I have refined comments to make it clear why we are not reading CRC for
> IEND chunk. Also chunkCRC value needs to be initialized because of this
> update, initialized value of chunkCRC will not be used anywhere in the logic.
>
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8211422/webrev.01/
>
> Thanks,
> Jay
>
> -Original Message-
> From: Sergey Bylokhov
> Sent: Wednesday, November 14, 2018 4:31 AM
> To: Jayathirth D V; 2d-dev@openjdk.java.net
> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
> corrupt CRC for IEND chunk throws IIOException
>
> Hi, Jay.
>
> Probably it will be better to not to change the code inside switch, and only
> add the check below around of reading CRC:
> "if (chunkType != IEND_TYPE) {"
>
> The current fix may throw "Invalid chunk length" when seek/flush the data for
> IEND_TYPE, which is not correct.
>
>
> On 12/11/2018 20:22, Jayathirth D V wrote:
>> Hello All,
>>
>> Gentle reminder for review.
>>
>> Thanks,
>>
>> Jay
>>
>> *From:*Jayathirth Rao
>> *Sent:* Tuesday, October 23, 2018 7:09 PM
>> *To:* 2d-dev@openjdk.java.net
>> *Subject:* [OpenJDK 2D-Dev] [12] RFR JDK-8211422: Reading PNG with
>> corrupt CRC for IEND chunk throws IIOException
>>
>> Hello All,
>>
>> Please review the following fix in JDK12:
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8211422
>>
>> Webrev: http://cr.openjdk.java.net/~jdv/8211422/webrev.00/
>>
>> Issue : When we try to read PNG image with corrupt/no 4 byte CRC data for
>> IEND chunk we throw IIOException. We see this issue only after JDK-8164971
>> .
>>
>> Root cause : In JDK-8164971
>> fix we made changes to
>> continue reading metadata until we reach IEND chunk. Before JDK-8164971
>> change we used to stop
>> reading metadata as soon as we hit first IDAT chunk. According to PNG spec
>> there can be more than one IDAT chunk/ more headers after IDAT chunk. So we
>> need to read metadata until we hit IEND chunk. But in case of this bug we
>> have IEND chunk but it has corrupt/no CRC chunk, so we throw
>> IIOException(According to PNG spec every PNG chunk should contain 4 byte
>> CRC). But lot of other decoders accept these kind of images which has no CRC
>> chunk for IEND chunk.
>>
>> Solution : There is no need to verify CRC for IEND chunk(Chunk data length
>> for IEND is 0). Stop reading metadata once we hit Chunk type info for IEND
>> chunk.
>>
>> Thanks,
>>
>> Jay
>>
>
>
> --
> Best regards, Sergey.
>
--
Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458
+1 Thanks, Krishna -Original Message- From: Jayathirth D V Sent: Tuesday, November 20, 2018 1:59 PM To: 2d-dev <2d-dev@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Thanks Sergey for the review. I need one more review. Please review the latest webrev: http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ Thanks, Jay -Original Message- From: Sergey Bylokhov Sent: Saturday, November 17, 2018 3:35 AM To: Jayathirth D V; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 Looks fine. On 16/11/2018 02:16, Jayathirth D V wrote: > Hi Sergey, > > As discussed offline I did more analysis on whether we can use common > variable to determine number of bands. Since we have "outputSampleSize.length > - 1" and "inputBands + 1" kind of things. > > Actually scale array will be used on input data(ps[]), so we should use input > bands value to create and use scale array. Before JDK-6788458 there was no > difference between input bands and output bands so we didn't see any issue. > But after JDK-6788458 number of bands data is different between input and > output data for PNG_COLOR_RGB/GRAY having tRNS chunk. > > So I have made change to use inputBands data for creation and use of scale > array. Ran complete imageio jtreg and JCK tests there are no failures. > Please find updated webrev for review: > http://cr.openjdk.java.net/~jdv/8211795/webrev.01/ > > Thanks, > Jay > > -Original Message- > From: Jayathirth D V > Sent: Wednesday, November 14, 2018 1:09 PM > To: Sergey Bylokhov; 2d-dev > Subject: RE: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi Sergey, > > Thanks for the review. > > As you pointed out, yes the AIOOB is happening for ps[b]. I have updated the > analysis in JBS bug also. > > Basically the calculation of numBands is not proper because we take numBands > value from destination image. This destination image will have extra alpha > channel for Gray or RGB input data(ps[]) which will throw AIOOB. > > So we need to update the logic of how we calculate numBands different for PNG > Gray/RGB image havng tRNS chunk. Fortunately, webrev.00 is actually doing > this job. > > Regarding whether we need to change scale array logic : We expect first 3 > channel to be RGB and first channel to be Gray for PNG_COLOR_RGB and > PNG_COLOR_GRAY respectively. So just updating numBands information will > create proper scale array. So there is no need to change the scale array > logic. > > History JDK-6788458 : Toolkit was able to show transparent color information > for RGB/Gray PNG image when it has tRNS chunk, but ImageIO didn't support it. > To use tRNS data and show transparent color in output image we needed to add > extra alpha channel for PNG RGB/Gray image with tRNS chunk. But fix present > in JDK-6788458 didn't handle the case where bitDepth adjustment is needed and > we are using band information from output image(having extra alpha channel) > on input image which has no alpha channel. Change in numBands logic for this > bug fixes that issue. > > > Thanks, > Jay > > -Original Message- > From: Sergey Bylokhov > Sent: Wednesday, November 14, 2018 4:07 AM > To: Jayathirth D V; 2d-dev > Subject: Re: [OpenJDK 2D-Dev] [12] RFR JDK-8211795: > ArrayIndexOutOfBoundsException in PNGImageReader after JDK-6788458 > > Hi, Jay. > > Can you please provide some more detail about this bug. > >> Root cause : In JDK-6788458 we are adding extra alpha channel for >> destination whenever we have tRNS chunk. But the number of bands in bitDepth >> scale array was not changed when we have tRNS chunk. This is causing >> ArrayIndexOutOfBoundsException for scale array. > > As far as I understand the AIOOB is occurred when we access ps[b] at line > 1308 not when we access the scale array, because the scale array is created > as "scale = new int[numBands][]". So maybe numBands should depends on the > passRow? or the creation of scale[][xxx] should be updated? > BTW this code uses +1/-1 in a lot of places already, and it is not always > clear why. > > -- Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] Review Request: 8212790 Javadoc cleanup of java.awt.color package
+1 Krishna > On 25-Oct-2018, at 10:06 AM, Sergey Bylokhov > wrote: > > Hi, Krishna. > Thank you for review! > >> 1894, 1895, 1902, 1904. > The text on the lines above is a block comment not a javadoc, the tags are > not necessary there. > >> 1570, 1601, > > I have fixed these: > http://cr.openjdk.java.net/~serb/8212790/webrev.01 > > -- > Best regards, Sergey.
Re: [OpenJDK 2D-Dev] [12] Review Request: 8212790 Javadoc cleanup of java.awt.color package
Hi Sergey,
I found a few places where @code tag should be added in ICC_Profile.java: 1570,
1601, 1894, 1895, 1902, 1904.
Thanks,
Krishna
> On 23-Oct-2018, at 4:50 AM, Sergey Bylokhov
> wrote:
>
> Hello.
> Please review a javadoc "weekend cleanup" in jdk 12.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8212790
> Webrev: http://cr.openjdk.java.net/~serb/8212790/webrev.00
> Specdiff:
> http://cr.openjdk.java.net/~serb/8212790/specdiff.00/overview-summary.html
>
> To check the difference I suggest to use specdiff.
>
> Some important changes:
> - The copyright header in CMMException.java is changed. I have checked the
> history and confirmed that this file had the same copyright as other files in
> jdk6. And I guess the header was not updated when it was moved to the open
> repo in JDK-6662775.
> - Descriptions of ICC_ProfileGray and ICC_ProfileRGB classes are unified.
> These are similar classes, but the javadoc was a little bit different. Any
> suggestions to improve it further are welcome.
>
> Other changes:
> - description of the class/method/field should be followed by dot
> - @param, @return should not end with a dot, except a case when more than one
> sentences are used
> - empty line after description/before the first tag was added
> - unnecessary empty lines were removed
> - sets of spaces in the middle of text were deleted
> - @param, @throws, @return should be aligned, to be more readable
> - unnecessary imports were removed
> - the "true"/"false" were wrapped in {@code } when necessary
> - @exception is replaced by @throws
>
> --
> Best regards, Sergey.
Re: [OpenJDK 2D-Dev] RFR: 8211962: Implicit narrowing in MacOSX java.desktop jsound
Hi Kim, An alternative way to fix it would be to declare the loop variable itself as unsigned, since the channels can never be negative, but that would require you to cast to int in two places. Also, when casting, it is better to use static_cast, rather than C style casting. Thanks, Krishna -Original Message- From: Brian Burkhalter Sent: Wednesday, October 10, 2018 4:53 AM To: Kim Barrett ; 2d-dev <2d-dev@openjdk.java.net> Cc: Java Core Libs Subject: Re: [OpenJDK 2D-Dev] RFR: 8211962: Implicit narrowing in MacOSX java.desktop jsound Looping in 2d-dev. > On Oct 9, 2018, at 4:17 PM, Kim Barrett wrote: > > [I'm not sure whether core-libs is the right list for java.desktop > changes.] > > Please review this trivial fix of a build failure on MacOSX when > compiling with C++11/14 enabled. An int value is being used in an > initializer where an unsigned int is needed, which is not permitted > since C++11. The solution taken is to cast the value in the > initializer. A "better" solution would be to change the type of the > value, but that has substantial fannout because there are many places > in our code where signed ints were used instead of the unsigned ints > used by the underlying MacOSX framework. > > CR: > https://bugs.openjdk.java.net/browse/JDK-8211962 > > Webrev: > http://cr.openjdk.java.net/~kbarrett/8211962/open.00/ > > Testing: > mach5 tier1 on MacOSX (really just verifying it builds). > >
Re: [OpenJDK 2D-Dev] [12]RFR: [JDK-8074824]: Resolve disabled warnings for libawt_xawt
Yes, that is right. I have compiled it Mac, Linux and Windows locally. I tried submitting a Mach5 job, but was unable to as it was down. Will try it again. Thanks Krishna > On 02-Oct-2018, at 3:39 AM, Philip Race wrote: > > I suspect I understand this one now .. the array is stack allocated so we > don't want NULL > but the compiler probably complained about possible uninitialised use of the > values ? > > -phil. > > On 10/1/18, 9:38 AM, Philip Race wrote: >> >> You really do need to explain *each* of the changes better. >> This one .. why not NULL ? >> http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html >> >> <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/share/native/libawt/java2d/loops/ProcessPath.c.udiff.html> >> >> -phil >> >> On 10/1/18, 9:19 AM, Philip Race wrote: >>> >>> Hi, >>> >>> 1) Has this been built on all platforms ? >>> 2) I can't find the list of warnings that you are seeing and fixing and >>> they are all over the place. >>> So adding 2d-dev and build-dev. >>> For each of these changes, please show what was the warning that you >>> received from the compiler >>> This might sound like a lot of work, but it won't be disproportionate and >>> I've made the same >>> request for similar reviews and without it, it is hard to review the >>> changes. >>> >>> For example (and I do mean just example) >>> http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html >>> >>> <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/src/java.desktop/unix/native/common/awt/awt_Font.c.udiff.html> >>> >>> why would that not be #ifdef instead ? >>> >>> 3) Testing .. did you run at least all our jtreg tests to make sure you >>> didn't break >>> some behaviour .. >>> >>> -phil. >>> >>> On 9/29/18, 8:18 PM, Krishna Addepalli wrote: >>>> >>>> Hi All, >>>> >>>> Please review a fix for JDK-8074824: >>>> https://bugs.openjdk.java.net/browse/JDK-8074824 >>>> <https://bugs.openjdk.java.net/browse/JDK-8074824> >>>> Webrev: http://cr.openjdk.java.net/~kaddepalli/8074824/webrev01/ >>>> <http://cr.openjdk.java.net/%7Ekaddepalli/8074824/webrev01/> >>>> >>>> Most of the warnings have been fixed for Linux, Mac and Windows. >>>> >>>> Thanks, >>>> Krishna
Re: [OpenJDK 2D-Dev] RFR: 8209548: Unused and incorrect calls to FT_Get_Char_Index
Looks fine! > On 20-Sep-2018, at 8:52 AM, Phil Race wrote: > > bug: https://bugs.openjdk.java.net/browse/JDK-8209548 > webrev: http://cr.openjdk.java.net/~prr/8209548/ > > Clean up of some pointless and incorrect code in freetypescaler.c > > -phil
Re: [OpenJDK 2D-Dev] RFR: 8210384: SunLayoutEngine.isAAT() font is expensive on MacOS
Looks good to me! Thanks Krishna > On 09-Sep-2018, at 1:21 AM, Philip Race wrote: > > bug: https://bugs.openjdk.java.net/browse/JDK-8210384 > webrev: http://cr.openjdk.java.net/~prr/8210384/ > > AAT is the Apple alternative to OpenType layout. > > Calls to the Harfbuzz layout engine need to pass a flag which says whether > the font is an AAT font > because we need to set up transforms differently. It is also looking like it > is needed for JDK 8 > with ICU because of problems handling AAT fonts there. > The current way of doing this via checking if there is a morx table is really > expensive. > So this fix adds a low-cost cache of the result. > No regression test because it is a performance fix but I did check the effect. > In a quick test rendering about 15 strings which broke up into 132 different > runs, > this reduced overall rendering time by 50% (and that includes the time to > draw the glyphs, > so it is an even bigger component of layout). > > -phil. > > > >
Re: [OpenJDK 2D-Dev] ArcIterator#btan(double)
Sorry, I replied assuming certain things before actually clarifying. Are you asking about the process for submitting a patch? Could you be a bit more specific so that I can answer better? -Original Message- From: Krishna Addepalli Sent: Tuesday, August 14, 2018 8:04 PM To: Ralph Hummeling ; 2d-dev@openjdk.java.net Cc: james.gra...@oracle.com Subject: Re: [OpenJDK 2D-Dev] ArcIterator#btan(double) I guess you would already have the code checked out? It is just a matter of updating the comments explaining how sin(a/2) / (1 + cos(a/2)) is equal to tan(a/4), and then change it in the code. I think you would need a test along with it. Thanks, Krishna -Original Message- From: Ralph Hummeling Sent: Tuesday, August 14, 2018 7:59 PM To: Krishna Addepalli ; 2d-dev@openjdk.java.net Cc: james.gra...@oracle.com Subject: Re: [OpenJDK 2D-Dev] ArcIterator#btan(double) Dear Krishna, I'd love to. How do I best do that? Kind regards, Ralph Hummeling +316 5758 1679 Hummeling Engineering BV https://urldefense.proofpoint.com/v2/url?u=http-3A__www.hummeling.com=DwICaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=QF7AertWDY_M4hfHg_4S-iyX-aP0wtLYwZFgs0zfX_k=iFb2N0ZBD0EAiVLz7h0dhj41Ha2uquc9ZM_GpTwq_w8=ypabzzNLUNIyzlXJK9-g27W-gz6a1phXmPa771N3ZGU= On 13/08/18 08:39, Krishna Addepalli wrote: > Dear Ralph, > > Thanks for pointing this out, could you also come up with a patch for the > same? > > Thanks, > Krishna > > -Original Message- > From: Ralph Hummeling > Sent: Friday, July 20, 2018 8:25 PM > To: 2d-dev@openjdk.java.net > Cc: james.gra...@oracle.com > Subject: [OpenJDK 2D-Dev] ArcIterator#btan(double) > > Dear Java 2D dev team, > > I came across your java.awt.geom.ArcIterator class in which an arc Bezier > control point segment length is determined in method btan(double). You arrive > at the following equation: > 4/3*(1 - cos(a/2))/(sin(a/2)) > > It is mentioned that this can return NaN for small "a" and so it is written > as: > 4/3*(sin(a/2))/(1 + cos(a/2)) > > Please note that this relation can also be written as follows: > 4/3*tan(a/4) > > A performance increase and no NaN issues ;-) > > > Kind regards, > > Ralph Hummeling > +316 5758 1679 > Hummeling Engineering BV > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.hummeling.com=DwICaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=QF7AertWDY_M4hfHg_4S-iyX-aP0wtLYwZFgs0zfX_k=iFb2N0ZBD0EAiVLz7h0dhj41Ha2uquc9ZM_GpTwq_w8=ypabzzNLUNIyzlXJK9-g27W-gz6a1phXmPa771N3ZGU= >
Re: [OpenJDK 2D-Dev] ArcIterator#btan(double)
I guess you would already have the code checked out? It is just a matter of updating the comments explaining how sin(a/2) / (1 + cos(a/2)) is equal to tan(a/4), and then change it in the code. I think you would need a test along with it. Thanks, Krishna -Original Message- From: Ralph Hummeling Sent: Tuesday, August 14, 2018 7:59 PM To: Krishna Addepalli ; 2d-dev@openjdk.java.net Cc: james.gra...@oracle.com Subject: Re: [OpenJDK 2D-Dev] ArcIterator#btan(double) Dear Krishna, I'd love to. How do I best do that? Kind regards, Ralph Hummeling +316 5758 1679 Hummeling Engineering BV https://urldefense.proofpoint.com/v2/url?u=http-3A__www.hummeling.com=DwICaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=QF7AertWDY_M4hfHg_4S-iyX-aP0wtLYwZFgs0zfX_k=iFb2N0ZBD0EAiVLz7h0dhj41Ha2uquc9ZM_GpTwq_w8=ypabzzNLUNIyzlXJK9-g27W-gz6a1phXmPa771N3ZGU= On 13/08/18 08:39, Krishna Addepalli wrote: > Dear Ralph, > > Thanks for pointing this out, could you also come up with a patch for the > same? > > Thanks, > Krishna > > -Original Message- > From: Ralph Hummeling > Sent: Friday, July 20, 2018 8:25 PM > To: 2d-dev@openjdk.java.net > Cc: james.gra...@oracle.com > Subject: [OpenJDK 2D-Dev] ArcIterator#btan(double) > > Dear Java 2D dev team, > > I came across your java.awt.geom.ArcIterator class in which an arc Bezier > control point segment length is determined in method btan(double). You arrive > at the following equation: > 4/3*(1 - cos(a/2))/(sin(a/2)) > > It is mentioned that this can return NaN for small "a" and so it is written > as: > 4/3*(sin(a/2))/(1 + cos(a/2)) > > Please note that this relation can also be written as follows: > 4/3*tan(a/4) > > A performance increase and no NaN issues ;-) > > > Kind regards, > > Ralph Hummeling > +316 5758 1679 > Hummeling Engineering BV > https://urldefense.proofpoint.com/v2/url?u=http-3A__www.hummeling.com=DwICaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=QF7AertWDY_M4hfHg_4S-iyX-aP0wtLYwZFgs0zfX_k=iFb2N0ZBD0EAiVLz7h0dhj41Ha2uquc9ZM_GpTwq_w8=ypabzzNLUNIyzlXJK9-g27W-gz6a1phXmPa771N3ZGU= >
Re: [OpenJDK 2D-Dev] ArcIterator#btan(double)
Dear Ralph, Thanks for pointing this out, could you also come up with a patch for the same? Thanks, Krishna -Original Message- From: Ralph Hummeling Sent: Friday, July 20, 2018 8:25 PM To: 2d-dev@openjdk.java.net Cc: james.gra...@oracle.com Subject: [OpenJDK 2D-Dev] ArcIterator#btan(double) Dear Java 2D dev team, I came across your java.awt.geom.ArcIterator class in which an arc Bezier control point segment length is determined in method btan(double). You arrive at the following equation: 4/3*(1 - cos(a/2))/(sin(a/2)) It is mentioned that this can return NaN for small "a" and so it is written as: 4/3*(sin(a/2))/(1 + cos(a/2)) Please note that this relation can also be written as follows: 4/3*tan(a/4) A performance increase and no NaN issues ;-) Kind regards, Ralph Hummeling +316 5758 1679 Hummeling Engineering BV www.hummeling.com
Re: [OpenJDK 2D-Dev] RFR: 8208466: Fix potential memory leak in harfbuzz shaping
+1 -Krishna -Original Message- From: Philip Race Sent: Saturday, July 28, 2018 11:38 PM To: 2d-dev <2d-dev@openjdk.java.net> Subject: [OpenJDK 2D-Dev] RFR: 8208466: Fix potential memory leak in harfbuzz shaping bug: https://bugs.openjdk.java.net/browse/JDK-8208466 webrev: http://cr.openjdk.java.net/~prr/8208466/ Simple fix for possible memory leaks on error conditions. This is the same fix as in upstream harfbuzz. -phil.
Re: [OpenJDK 2D-Dev] [11] Review Request: 8201611 Broken links in java.desktop javadoc
Looks fine -Original Message- From: Phil Race Sent: Monday, July 2, 2018 10:46 PM To: Sergey Bylokhov ; awt-...@openjdk.java.net; 2d-dev <2d-dev@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] [11] Review Request: 8201611 Broken links in java.desktop javadoc OK, +1 -phil. On 07/02/2018 10:09 AM, Sergey Bylokhov wrote: > On 02/07/2018 09:29, Phil Race wrote: >> Not exactly, it was after javadoc was changed under >> https://bugs.openjdk.java.net/browse/JDK-8195795 > > So modules are added to the path. > >> This one seems to be different than the other two .. no mention of >> the module >> http://cr.openjdk.java.net/~serb/8201611/webrev.00/src/java.desktop/s >> hare/classes/javax/imageio/metadata/IIOMetadataNode.java.udiff.html > > > The html tag was replaced by the direct javadoc link to the java > class. > >> Why is it correct ? >> >> -phil. >> >> >> On 07/02/2018 09:11 AM, Sergey Bylokhov wrote: >>> Hello. >>> Please review the fix for jdk11. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201611 >>> Webrev: http://cr.openjdk.java.net/~serb/8201611/webrev.00 >>> >>> Some links in the javadoc became broken after modules were added. >>> >> > >
Re: [OpenJDK 2D-Dev] RFR: 8204187: Remove proprietary JPEG code from javax.imageio
Hi Phil,
The changes look fine to me. However I have one question: In
SOFMarkerSegment.java file, in the function below,
int getIDencodedCSType () {
for (int i = 0; i < componentSpecs.length; i++) {
if (componentSpecs[i].componentId < 'A') {
return JPEG.JCS_UNKNOWN;
}
}
switch(componentSpecs.length) {
case 3:
if ((componentSpecs[0].componentId == 'R')
&&(componentSpecs[0].componentId == 'G')
&&(componentSpecs[0].componentId == 'B')) {
return JPEG.JCS_RGB;
}
componentId is an integer, and it can store only one value R, G or B. However,
the condition checks for the integer value being equal to all the three values
simultaneously which is impossible.
I know this code was existing before your changes, but not sure if my
understanding is correct.
Thanks,
Krishna
-Original Message-
From: Phil Race
Sent: Friday, June 1, 2018 10:35 PM
To: 2d-dev <2d-dev@openjdk.java.net>
Subject: [OpenJDK 2D-Dev] RFR: 8204187: Remove proprietary JPEG code from
javax.imageio
Bug : https://bugs.openjdk.java.net/browse/JDK-8204187
CSR : https://bugs.openjdk.java.net/browse/JDK-8204189
Webrev : http://cr.openjdk.java.net/~prr/8204187/
Please review the code and the CSR.
There are 4 optional color spaces for the standard JPEG plugin documented in
the javax.image I/O JPEG metadata specification.
https://docs.oracle.com/javase/10/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#color
---
Optional ColorSpace support: Handling of PhotoYCC (YCC), PhotoYCCA (YCCA), RGBA
and YCbCrA color spaces by the standard plugin, as described below, is
dependent on capabilities of the libraries used to interpret the JPEG data.
Thus all consequential behaviors are optional.
..
..
[and more details]
---
The necessary support for these is implemented in a modified, closed source,
IJG libjpeg 6b.
What this means is that OpenJDK never has supported these.
However code to handle these on the Java side is implemented in the open code
Oracle intends to remove the closed source native support, so we can clean up
the open code.
And there's a particular bug that even the supported YCCK color space would not
work with OpenJDK due to the ID specified in the Java code not matching the ID
specified in the standard IJG libraries.
Another fix is that even though it did not have support for writing images with
alpha, the writer would say it could - true only for a JDK with the closed
enhancements.
So what would happen is that even if code would check to see if it was
supported when it tried it would get this exception :
Exception in thread "main" javax.imageio.IIOException: Invalid argument to
native writeImage
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.writeImage(Native
Method)
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.writeOnThread(JPEGImageWriter.java:1075)
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.write(JPEGImageWriter.java:371)
at java.desktop/javax.imageio.ImageWriter.write(ImageWriter.java:613)
at java.desktop/javax.imageio.ImageIO.doWrite(ImageIO.java:1628)
at java.desktop/javax.imageio.ImageIO.write(ImageIO.java:1594)
at TestWriteARGBJPEG.main(TestWriteARGBJPEG.java:15)
With the removal of the Java code related to these color spaces, you will still
get an exception, but a slightly different one :
Exception in thread "main" javax.imageio.IIOException: Bogus input colorspace
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.writeImage(Native
Method)
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.writeOnThread(JPEGImageWriter.java:1007)
at
java.desktop/com.sun.imageio.plugins.jpeg.JPEGImageWriter.write(JPEGImageWriter.java:371)
at java.desktop/javax.imageio.ImageWriter.write(ImageWriter.java:613)
at TestWriteARGBJPEG.main(TestWriteARGBJPEG.java:23)
The key to avoiding both of these is making sure the code that checks support
looks to see if there is an alpha channel and gives you a correct answer. That
is fixed.
I have also included a new test which covers this and updated an existing one.
I am leaving the specification alone for now at least, to leave the door open
to reinstating such support somehow if it there is a need. If we do it may be
just a subset.
I am preparing a release note as well as a CSR for this.
-phil.
Re: [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata Background color initialization from standard metadata is incomplete
Hi Jay, Here are my observations/questions: 1. I see that the fix is to update the bKGD_colorType for Background color child. However, don't you need to do the same thing for paletted images as well? 2. As for the test case, I'm not sure how ImageReaders and ImageWriters work. I see that you query them from ImageIO, so the question is, do you need to get a new ImageReader/ImageWriter for each image read/write operation? If so, then, there is a potential problem when you call VerifyNativeRGBValuesFromBKGDChunk() and VerifyStandardRGBValuesFromBKGDChunk() in succession, since each in turn calls writeAndReadMetaData() and the writer is disposed. On the other hand, if querying for the reader/writer for once is enough, then you can initialize the image reader/writer in the static block itself, and then run the whole test. Thanks, Krishna From: Jayathirth D V Sent: Wednesday, April 18, 2018 3:34 PM To: 2d-dev Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata Background color initialization from standard metadata is incomplete Hello All, Since changes related to JDK-6574555 is pushed. Please find new webrev which captures test case changes over JDK-6574555. http://cr.openjdk.java.net/~jdv/5109146/webrev.01/ Thanks, Jay From: Jayathirth D V Sent: Tuesday, April 17, 2018 3:34 PM To: 2d-dev Subject: [OpenJDK 2D-Dev] [11] RFR JDK-5109146: PNGMetadata Background color initialization from standard metadata is incomplete Hello All, Please review the following fix in JDK11 : Bug : https://bugs.openjdk.java.net/browse/JDK-5109146 Webrev : http://cr.openjdk.java.net/~jdv/5109146/webrev.00/ Issue: PNGMetadata.mergeStandardTree() function doesn't set proper bKGD colortype. Solution: When bKGD R, G, B values are unique we need to store appropriate bKGD colortype in metadata. Note : Test case which is present as part of review in JDK-6574555 is only updated to handle regression scenario for this bug also. Regression scenario between this bug and JDK-6574555 differ only in what type(native/standard) metadata we use to merge bKGD RGB values. Thanks, Jay
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hmmm, thanks for the clarification, but this raises one more question: Now that
you are initializing the colorType to an invalid value, do you need to check
appropriately in all the places where the color is being used? Otherwise, it
might lead to undefined behavior.
Also, I would like you to either add a comment at the point of initialization
or better yet, define another static constant of "UNDEFINED", and then set the
tRNS_colorType to this value, so that the code reads correct naturally without
any comments.
Thanks,
Krishna
From: Jayathirth D V
Sent: Monday, April 2, 2018 11:33 AM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
The constant values for different color types is :
static final int PNG_COLOR_GRAY = 0;
static final int PNG_COLOR_RGB = 2;
static final int PNG_COLOR_PALETTE = 3;
static final int PNG_COLOR_GRAY_ALPHA = 4;
static final int PNG_COLOR_RGB_ALPHA = 6;
Since tRNS_colorType is used to hold one of the above mentioned values if we
don't initialize it to some unused value(here we are using -1) we will be
representing PNG_COLOR_GRAY by default.
By default the value will be -1 after the change and after we parse tRNS chunk
it will contain appropriate value. The initialization of tRNS_colorType change
can be considered as newly added check.
Thanks,
Jay
From: Krishna Addepalli
Sent: Monday, April 02, 2018 9:56 AM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli mailto:krishna.addepa...@oracle.com"krishna.addepa...@oracle.com>; 2d-dev
mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Jay,
I have s
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Jay,
The changes look fine. However, I have one more question: Why did you have to
explicitly specify the initial value to "tRNS_colorType" in PNGMetadata.java?
How is it affecting your implementation?
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 1:43 PM
To: Krishna Addepalli <krishna.addepa...@oracle.com>; 2d-dev
<2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Krishna,
Thanks for providing the inputs.
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
As discussed I have added new name as isTransparentPixelPresent()
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
Changes are made.
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
Previous to this change all the values like inputsBands, bytesPerRow,
eltsPerRow were same between the decoded output and destination image.
Now because we are changing only the destination image due to the presence of
transparent pixel, we need both these set of values. inputsBands, bytesPerRow,
eltsPerRow will be used for decoded output and destBands, destEltsPerRow for
destination image. Its better if don't mingle code flow or variables between
these two code paths.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
Changes are made.
Please find updated webrev for review :
http://cr.openjdk.java.net/~jdv/6788458/webrev.03/
Thanks,
Jay
From: Krishna Addepalli
Sent: Wednesday, March 28, 2018 11:52 AM
To: Jayathirth D V; 2d-dev
Subject: RE: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
I haven't yet checked the test cases.
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 9:52 AM
To: 2d-dev mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: P
Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS chunk while reading non-indexed RGB images
Hi Jay,
I have some points as below:
1. I suggest to rename considerTransparentPixel as isAlphaPresent.
2. You can refactor the function body as below:
Return ((destinationBands == null ||
destinationBands.length == 4) &&
metadata.tRNS_colorType == PNG_COLOR_RGB);
3. From line 1088 through 1113, I see some repeated calculations like
bytesPerRow etc. Why can't we reuse the previously defined variables? Apart
from destBands, all the other calculations look similar and repeated.
4. When you are copying the pixels to a writable raster, at line 1273,
you could reduce the repetition of the lines, by just having one statement
inside if as below:
for (int i = 0; i < passWidth; i++) {
byteData[destidx] = curr[srcidx];
byteData[destidx + 1] = curr[srcidx + 1];
byteData[destidx + 2] = curr[srcidx + 2];
if (curr[srcidx] == (byte)metadata.tRNS_red &&
curr[srcidx + 1] == (byte)metadata.tRNS_green &&
curr[srcidx + 2] == (byte)metadata.tRNS_blue)
{
byteData[destidx + 3] = (byte)0;
} else {
byteData[destidx + 3] = (byte)255;
}
srcidx += 3;
destidx += 4;
}
Same thing can be done for the loop that executes for 16bit pixel data.
I haven't yet checked the test cases.
Thanks,
Krishna
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 9:52 AM
To: 2d-dev <2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
I have enhanced both the test case to verify pixels which not only match tRNS
transparent pixel data but also for them which doesn't match tRNS transparent
pixel data.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.02/
Thanks,
Jay
From: Jayathirth D V
Sent: Wednesday, March 28, 2018 8:28 AM
To: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
I just realized that the test case Read16BitPNGWithTRNSChunk.java is creating
(50, 50) image(which was done while debugging the issue), which is not needed
as we need single pixel to verify the fix as it is done in
Read8BitPNGWithTRNSChunk.java. I have updated Read16BitPNGWithTRNSChunk.java to
reflect this small change.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/6788458/webrev.01/
Thanks,
Jay
From: Jayathirth D V
Sent: Tuesday, March 27, 2018 6:51 PM
To: 2d-dev
Subject: [OpenJDK 2D-Dev] [11] RFR JDK-6788458: PNGImageReader ignores tRNS
chunk while reading non-indexed RGB images
Hello All,
Please review the following solution in JDK11 :
Bug : https://bugs.openjdk.java.net/browse/JDK-6788458
Webrev : http://cr.openjdk.java.net/~jdv/6788458/webrev.00/
Issue: When we try to read non-indexed RGB PNG image having transparent pixel
information in tRNS chunk, ImageIO.PNGImageReader ignores the transparent pixel
information. If we use Toolkit.getDefaultToolkit().createImage() it doesn't
ignore the transparent pixel information.
Root cause: In ImageIO.PNGImageReader() we store tRNS chunk values in
readMetadata() -> parse_tRNS_chunk (), but we never use the tRNS R, G, B value
to compare with decoded image information. Also in ImageIO.PNGImageReader() for
IHDR colortype RGB we use only 3 channel destination BufferedImage. So even if
we use the tRNS chunk value we need destination BufferedImage of 4 channels to
represent transparency.
Solution: While assigning destination image in PNGImageReader.getImageTypes()
if we know that PNG image is of type RGB and has tRNS chunk then we need to
assign a destination image having 4 channels. After that in decodePass() we
need to create 4 channel destination raster and while we store decoded image
information into the destination BufferedImage we need to compare decoded R, G,
B values with tRNS R, G, B values and store appropriate alpha channel value.
Also we use 4 channel destination image in case of RGB image only when tRNS
chunk is present and ImageReadParam.destinationBands is null or
ImageReadParam.destinationBands is equal to 4.
One more change is that we have optimization in PNGImageReader.readMetadata()
where if ignoreMetadata is true and IHDR colortype is not of type
PNG_COLOR_PALETTE, we ignore all the chunk data and just try to find first IDAT
chunk. But if we need tRNS chunk values in case of IHDR colortype PNG_COLOR_RGB
we need to parse tNRS chunk also. So in readMetadata() also appropriate changes
are made.
Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values
Hi Prahalad, The changes look fine to me. Thanks, Krishna -Original Message- From: Prahalad Kumar Narayanan Sent: Friday, March 23, 2018 1:40 PM To: 2d-dev <2d-dev@openjdk.java.net> Subject: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values Hello Everyone Good day to you. Request your time to review a simple fix for the bug Bug: JDK-4954348 (https://bugs.openjdk.java.net/browse/JDK-4954348) Title: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values Root Cause: . As per ImageWriter specification, the method- getNumThumbnailsSupported, should return -1 when invoked with insufficient data. . However, the method's implementation in JpegImageWriter returns 0 instead of -1 in this use-case. Information on the fix: . The logic within concerned method of Jpeg image writer requires one of the two inputs- ImageTypeSpecifer (or) IIOMetadata. . If both the required inputs are 'Null' the method has insufficient data & thus cannot validate. . A simple check is now added in the method to detect this use-case and return -1 as recommended in the specification. Kindly review the changes at your convenience Link: http://cr.openjdk.java.net/~pnarayanan/4954348/webrev.00 Thank you Have a good day Prahalad N.
