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] 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] Speed of drawPolyline on JDK11
Hi,
As OpenJDK12 RDP1 is coming soon, I propose this plan:
- integrate this basic fix in ShapeSpanIterator.c code to use stdlib sort
(mergesort on linux)
- integrate a very simple patch in Marlin renderer to disable insertion
sort for large arrays: 0.5s to 0.25s, few LOC
- postpone my changes to Marlin sort & Marlin nonAA renderer integration in
OpenJDK 13
Will you have time to review 2 small patchs on time ?
Cheers,
Laurent
Le mar. 23 oct. 2018 à 22:37, Laurent Bourgès a
écrit :
> Phil,
> I quickly modified the final update & sort loop to:
> - move sort in another block
> - use qsort() using a new comparator sortSegmentsByCurX
>
> This improves performance in PolyLineTest by 3 times: ~1s vs 3.5s !
> Apparently qsort() is not optimal (comparator can not be inlined by c) so
> it may explain why Marlin (0x0 sampling) is still 2 times faster with its
> custom merge-sort (in-place).
>
> Any idea to improve C sort ?
> Is it good enough ?
>
> - USE_QSORT_X: 1
> oct. 23, 2018 10:15:29 PM polylinetest.Canvas paintComponent
> INFOS: Paint Time: 1,081s
> INFOS: Paint Time: 1,058s
> INFOS: Paint Time: 1,067s
>
> - USE_QSORT_X: 0
> oct. 23, 2018 10:18:50 PM polylinetest.Canvas paintComponent
> INFOS: Paint Time: 3,318s
> INFOS: Paint Time: 3,258s
> INFOS: Paint Time: 3,273s
>
> Patch:
> diff -r 297450fcab26
> src/java.desktop/share/native/libawt/java2d/pipe/ShapeSpanIterator.c
> ---
> a/src/java.desktop/share/native/libawt/java2d/pipe/ShapeSpanIterator.c
> Tue Oct 16 23:21:05 2018 +0530
> +++
> b/src/java.desktop/share/native/libawt/java2d/pipe/ShapeSpanIterator.c
> Tue Oct 23 22:31:00 2018 +0200
> @@ -1243,6 +1243,18 @@
> }
> }
>
> +/* LBO: enable (1) / disable (0) qsort on curx */
> +#define USE_QSORT_X (0)
> +
> +static int CDECL
> +sortSegmentsByCurX(const void *elem1, const void *elem2)
> +{
> +jint x1 = (*(segmentData **)elem1)->curx;
> +jint x2 = (*(segmentData **)elem2)->curx;
> +
> +return (x1 - x2);
> +}
> +
> static jboolean
> ShapeSINextSpan(void *state, jint spanbox[])
> {
> @@ -1378,16 +1390,28 @@
> seg->curx = x0;
> seg->cury = y0;
> seg->error = err;
> +}
>
> -/* Then make sure the segment is sorted by x0 */
> -for (new = cur; new > lo; new--) {
> -segmentData *seg2 = segmentTable[new - 1];
> -if (seg2->curx <= x0) {
> -break;
> +if (USE_QSORT_X && (hi - lo) > 100)
> +{
> +/* use quick sort on [lo - hi] range */
> +qsort(&(segmentTable[lo]), (hi - lo), sizeof(segmentData *),
> +sortSegmentsByCurX);
> +} else {
> +for (cur = lo; cur < hi; cur++) {
> +seg = segmentTable[cur];
> +x0 = seg->curx;
> +
> +/* Then make sure the segment is sorted by x0 */
> +for (new = cur; new > lo; new--) {
> +segmentData *seg2 = segmentTable[new - 1];
> +if (seg2->curx <= x0) {
> +break;
> +}
> +segmentTable[new] = seg2;
> }
> -segmentTable[new] = seg2;
> +segmentTable[new] = seg;
> }
> -segmentTable[new] = seg;
> }
> cur = lo;
> }
>
> Cheers,
> Laurent
>
> Le mar. 23 oct. 2018 à 08:30, Laurent Bourgès
> a écrit :
>
>> Phil,
>> Yesterday I started hacking ShapeSpanIterator.c to add stats: the last
>> stage (sort by x0) is the bottleneck.
>> In this case every sort takes up to 15ms per pixel row !
>>
>> I will see if I can adapt Marlin's MergeSort.java to C to have an
>> efficient sort in-place.
>> Do you know if libawt has already an efficient sort instead of porting
>> mine ?
>>
>> PS: "To save the planet, make software more efficient" is my quote of the
>> day !
>>
>> Cheers,
>> Laurent
>>
>
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] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
On 2018-11-19 18:56, Volker Simonis wrote: Hi Phil, I'd like to kindly ask you to suggest how we can proceed with this issue. As I wrote before, Xrandr is not officially supported on AIX and there are no official packages available for it. There are some OpenSource sites for AIX which provide Xrandr, but they are all not compatible with the default native libraries (e.g. the open source Xrandr package depends on another open source package of Xrender.so.1 but the system only provides Xrender.so.0 natively). We can't compile the whole JDK (i.e. libawt_xawt.so) against some open source package of Xrender.so.1 because that simply won't be available on the majority of systems. Remember that forcing people to install these open-source packages even as a build dependency is like expecting Linux users to install some non-official packages just to be able to build. Especially in corporate environments that's not easy. Moreover the benefit would be really minimal, because the Xrandr functionality won't be available at runtime anyway. So to cut a long story short, I see two options: 1. Go with my current patch (ugly but efficient) 2. Check-in in an AIX specific version of XRander.h/randr.h under src/java.desktop/aix (OK for me, but that would actually negate the initial purpose of "8210863: Remove Xrandr include files") Do you have a better proposal? I think the change look good, and I vote for strategy 1. As Thomas suggested, if the AIX ifdefs look bad we can create a new define, but I'm not sure that's really helpful - after all, it's just on AIX we currently have no r Having a define would mostly be needed if it was multiple OSes, or similar more complex situations, that would have/not have the r extension. Yet another solution, to get rid of the ifdefs, is to move the relevant Xranrd dependent functions into a new, separate file, like awt_GraphicsEnv_randr.c, and then in the build exclude it on AIX (or, perhaps if it's worth the trouble, on all platforms where configure did not find Xrandr). /Magnus Thank you and best regards, Volker On Fri, Nov 16, 2018 at 11:22 AM Volker Simonis wrote: On Thu, Nov 15, 2018 at 6:01 PM Philip Race wrote: PS I am not sure why xrandr headers would not be available for AIX. They are a standard part of the xdistribution. I'm not an X11 guru, but as far as I understand, xrandr is an extension and as such it doesn't have to be supported by every implementation. To the best of my knowledge (I've just started another poll among some experts) AIX doesn't support Xrandr and does not have the corresponding headers. If true, think what you are going to have to do is add a --with-xrandr-include option and provide it that way. What if there are no standard Xrandr headers on a platform? Do you really want to force users to get them from some dubious sources just for building the OpenJDK? Sorry, but I don't think that's a good solution. Than I'd rather prefer the ugly ifdefs (or I check the headers back in again in an AIX-specific directory :) Thank you and best regards, Volker -phil. On 11/15/18, 8:55 AM, Philip Race wrote: Hmm. I don't like the ifdefs. Xrandr is a requirement for the build. If its not there at runtime that's OK. -phil. On 11/15/18, 8:06 AM, Volker Simonis wrote: Hi, can I please have a review for the following small change: http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ https://bugs.openjdk.java.net/browse/JDK-8213944 Change JDK-8210863 removed the Xrandr.h/randr.h headers from the OpenJDK sources but forgot to add a configure check for the Xrandr extension which is now a build dependency. The change also broke the AIX build. AIX never supported Xrandr, but that was only detected at runtime, when the JDK was unable to dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the source tree any more, we have to conditionally compile some parts of src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such that it doesn't require the definitions and declarations from Xrandr.h/randr.h any more. Thank you and best regards, Volker
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
>> .
>>
>> 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
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
Hi Volker, I had a look at your change. It looks good. I appreciate a lot you added a check in configure. Maybe it would be better to pass a WITHOUT_XRANDR or the like to the build, and check for such a define in the code. But I think we should push this change for now to fix the build. A follow up still can re-enable xrandr on AIX again or prettify the code. Best regards, Goetz. > -Original Message- > From: 2d-dev <2d-dev-boun...@openjdk.java.net> On Behalf Of Philip Race > Sent: Thursday, November 15, 2018 6:02 PM > To: Volker Simonis > Cc: 2d-dev <2d-dev@openjdk.java.net>; build-dev d...@openjdk.java.net> > Subject: Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the > removal of Xrandr.h and add a configure check for it > > PS I am not sure why xrandr headers would not be available for AIX. > They are a standard part of the xdistribution. > > If true, think what you are going to have to do is add a > --with-xrandr-include option > and provide it that way. > > -phil. > > On 11/15/18, 8:55 AM, Philip Race wrote: > > Hmm. I don't like the ifdefs. > > > > Xrandr is a requirement for the build. If its not there at runtime > > that's OK. > > > > -phil. > > > > On 11/15/18, 8:06 AM, Volker Simonis wrote: > >> Hi, > >> > >> can I please have a review for the following small change: > >> > >> http://cr.openjdk.java.net/~simonis/webrevs/2018/8213944/ > >> https://bugs.openjdk.java.net/browse/JDK-8213944 > >> > >> Change JDK-8210863 removed the Xrandr.h/randr.h headers from the > >> OpenJDK sources but forgot to add a configure check for the Xrandr > >> extension which is now a build dependency. > >> > >> The change also broke the AIX build. AIX never supported Xrandr, but > >> that was only detected at runtime, when the JDK was unable to > >> dynamically load libXrand.so. Now, without Xrandr.h/randr.h in the > >> source tree any more, we have to conditionally compile some parts of > >> src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c such > >> that it doesn't require the definitions and declarations from > >> Xrandr.h/randr.h any more. > >> > >> Thank you and best regards, > >> Volker
Re: [OpenJDK 2D-Dev] Speed of drawPolyline on JDK11
On Tue, Nov 20, 2018 at 8:28 AM Laurent Bourgès wrote: > Will you have time to review 2 small patchs on time ? If it would help for me to have a look, I am happy to do so. Pete
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
>> .
>>
>> 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] Gcc 8.1 HarfBuzz library compilation issues
I have removed awt-dev and replaced it with the CORRECT 2d-dev list. Please reply to this one .. to get awt-dev out of the loop. I already have an open bug to upgrade harfbuzz I will be handling soon. If the necessary changes are there we are all good. We do not much like at all locally modifying sources we get from upstream. It is a maintenance nightmare. -phil On 11/20/18, 6:48 AM, Simon Tooke wrote: On 2018-11-20 5:31 a.m., Magnus Ihse Bursie wrote: On 2018-11-19 22:22, Simon Tooke wrote: Hello, I've been looking at compiling the JDK with GCC 8.1, and trying to fix issues upstream as I find them. Nice! If you find issues in the JDK source code per se, please let us know. :) Sooner or later, we'll have to support gcc 8 properly anyway, might just as well try to clear the path already. I noticed yesterday that https://bugs.openjdk.java.net/browse/JDK-8213153 from Dmitry Chuyko is also working on GCC 8. I am not sure how far down the rabbit hole he is, but I (with the kind assistance of Severin Gehwolf since I'm not a committer) am submitting a series of patches to move towards that. I have a JDK tree compiling with 8.1, but it still has to disable warnings in more places than I feel comfortable with. /Magnus For example the version of HarfBuzz shipped in the JDK has some issues with template specialization and with clearing a struct via memset() (thus bypassing C++ member access restrictions). I've had changes to address these issues accepted upstream [1], and I was wondering if it is appropriate for me to file an issue to backport those fixes (I have a patch ready) or do you prefer to wait until HarfBuzz is upgraded to the latest version during the natural course of JDK development. (Be aware that the JDK is on a 1.7 version, while these fixes are in a 2.X version). Thanks, -Simon [1] https://github.com/harfbuzz/harfbuzz/pull/1334
Re: [OpenJDK 2D-Dev] RFR(XS): 8213944: Fix AIX build after the removal of Xrandr.h and add a configure check for it
Thanks everybody for the reviews. If nobody raises a "Veto" (Phil?) I plan to push this fix tomorrow in its current form. I've also run it through the submit repo and got an error on Windows for the test "runtime/modules/JVMDefineModule.java" which seems completely unrelated to my change which only touches the X11 implementation on Unix. Can somebody please confirm that? [Mach5] mach5-one-simonis-JDK-8213944-20181120-1629-11082: FAILED, Failed tests: 1 runtime/modules/JVMDefineModule.java tier1 windows-x64-debug othervm driver ExitCode: -1073741819 Mach5 Tasks Results Summary UNABLE_TO_RUN: 0 FAILED: 0 EXECUTED_WITH_FAILURE: 1 KILLED: 0 PASSED: 75 NA: 0 Thank you and best regards, Volker On Tue, Nov 20, 2018 at 12:05 PM Magnus Ihse Bursie wrote: > > > > On 2018-11-19 18:56, Volker Simonis wrote: > > Hi Phil, > > > > I'd like to kindly ask you to suggest how we can proceed with this issue. > > > > As I wrote before, Xrandr is not officially supported on AIX and there > > are no official packages available for it. There are some OpenSource > > sites for AIX which provide Xrandr, but they are all not compatible > > with the default native libraries (e.g. the open source Xrandr package > > depends on another open source package of Xrender.so.1 but the system > > only provides Xrender.so.0 natively). We can't compile the whole JDK > > (i.e. libawt_xawt.so) against some open source package of Xrender.so.1 > > because that simply won't be available on the majority of systems. > > > > Remember that forcing people to install these open-source packages > > even as a build dependency is like expecting Linux users to install > > some non-official packages just to be able to build. Especially in > > corporate environments that's not easy. Moreover the benefit would be > > really minimal, because the Xrandr functionality won't be available at > > runtime anyway. > > > > So to cut a long story short, I see two options: > > > > 1. Go with my current patch (ugly but efficient) > > > > 2. Check-in in an AIX specific version of XRander.h/randr.h under > > src/java.desktop/aix (OK for me, but that would actually negate the > > initial purpose of "8210863: Remove Xrandr include files") > > > > Do you have a better proposal? > I think the change look good, and I vote for strategy 1. As Thomas > suggested, if the AIX ifdefs look bad we can create a new define, but > I'm not sure that's really helpful - after all, it's just on AIX we > currently have no r Having a define would mostly be needed if it was > multiple OSes, or similar more complex situations, that would have/not > have the r extension. > > Yet another solution, to get rid of the ifdefs, is to move the relevant > Xranrd dependent functions into a new, separate file, like > awt_GraphicsEnv_randr.c, and then in the build exclude it on AIX (or, > perhaps if it's worth the trouble, on all platforms where configure did > not find Xrandr). > > /Magnus > > > > > Thank you and best regards, > > Volker > > > > On Fri, Nov 16, 2018 at 11:22 AM Volker Simonis > > wrote: > >> On Thu, Nov 15, 2018 at 6:01 PM Philip Race wrote: > >>> PS I am not sure why xrandr headers would not be available for AIX. > >>> They are a standard part of the xdistribution. > >>> > >> I'm not an X11 guru, but as far as I understand, xrandr is an > >> extension and as such it doesn't have to be supported by every > >> implementation. To the best of my knowledge (I've just started another > >> poll among some experts) AIX doesn't support Xrandr and does not have > >> the corresponding headers. > >> > >>> If true, think what you are going to have to do is add a > >>> --with-xrandr-include option > >>> and provide it that way. > >>> > >> What if there are no standard Xrandr headers on a platform? Do you > >> really want to force users to get them from some dubious sources just > >> for building the OpenJDK? Sorry, but I don't think that's a good > >> solution. Than I'd rather prefer the ugly ifdefs (or I check the > >> headers back in again in an AIX-specific directory :) > >> > >> Thank you and best regards, > >> Volker > >> > >>> -phil. > >>> > >>> On 11/15/18, 8:55 AM, Philip Race wrote: > >>>> Hmm. I don't like the ifdefs. > >>>> > >>>> Xrandr is a requirement for the build. If its not there at runtime > >>>> that's OK. > >>>> > >>>> -phil. > >&g
