• Type: Bug
• Component: javafx
• Sub-Component: graphics
• Affected Version: 7u181,8,openjfx11

• Priority: P3
• Status: Resolved
• Resolution: Fixed
• OS: generic
• CPU: generic

• Submitted: 2018-05-28
• Updated: 2022-11-23
• Resolved: 2018-10-12

JDK 7	JDK 8	Other
7u211Fixed	8u201Fixed	openjfx11.0.2Fixed

There are no substantive differences that would preclude this backport from being pushed. Ambarish: go ahead and push the backport. Then please file a follow-up issue to address Jay's comment. 15-10-2018 Changes related to 8u Backport is also fine +1. But I just noticed one problem in modules/javafx.graphics/src/main/legal/jpeg_fx.md. It looks like we are picking license from wrong file like jcapimin.c which might not be updated in every version. We should pick latest license text from files like jversion.h which are sure to be updated with each revision and use it in jpeg_fx.md. 15-10-2018 Approved to backport to 8u-dev for 8u202. +1 to the 8u .00 webrev 13-10-2018 Hi Kevin & Jay, Please take look at the 8u-dev back port change set : http://cr.openjdk.java.net/~arapte/fx/8203884/8u-dev/webrev.00/ Verified build & all test run on Windows 7, Mac 10.13.3, Ubuntu 16.04 No difference is observed in test execution before and after the update of libjpeg library from 7 to 9c 12-10-2018 Author: arapte Date: 2018-10-11 11:37 +0530 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/dc4eac151944 Reviewed-by: kcr, jdv Contributed-by: jayathirth.d.v@oracle.com 12-10-2018 [~jdv] Good point about capturing changes we make. Some sort of README or CHANGES file in the repo might be a good way to go. Let's file a follow-on issue to track this, so as not to hold up getting this in and back-ported. 10-10-2018 We need to capture the incremental changes that we are making over the IJG libjpeg 9c code, which will be useful reference for future updates. Incremental changes we are making are : 1) Explicitly declare boolean definition in jconfig.h to override the logic present in jmorecfg.h 2) Arithmetic coding references present in jcinit.c, jdtrans.c, jdmaster.c, jctrans.c are replaced with error messages. Corresponding error message definition for JERR_ARITH_NOTIMPL is added in jerror.h from JavaFX libjpeg7 implementation. webrev.02 changes are fine. 10-10-2018 The .02 version looks good to me. Please file a follow-up issue to address the additional memory free calls in jpegloader.c +1 09-10-2018 Hi Kevin & Jay, As discussed with Kevin, we shall check-in only libjpeg9c update changes under this issue. Please take a look at the updated webrev: http://cr.openjdk.java.net/~arapte/fx/8203884/webrev.02/ 09-10-2018 Hi Kevin, Jay, Ajit, Please review the fix for this issue, Webrev: http://cr.openjdk.java.net/~arapte/fx/8203884/webrev.00_for_checkin/ -> Complete patch with changes in make files and Netbeans project files. Webrev: http://cr.openjdk.java.net/~arapte/fx/8203884/webrev.00_diff_with_libjpeg7/ -> Partial patch, updated 9c files in existing libjpeg7 folder for ease to review difference. Also added libjpeg9c files can be compared with IJG libjpeg9c original source, to identify the minor changes we require for integrating with FX. Additionally the changes include some memory free calls in error case. changes are only in one file: jpegloader.c http://cr.openjdk.java.net/~arapte/fx/8203884/webrev.00_for_checkin/modules/javafx.graphics/src/main/native-iio/jpegloader.c.sdiff.html Verification: Executed all tests & Ensemble app on Win7, Ubuntu16.04 and Mac. 09-10-2018 Review for webrev.01: Overall the changes are fine. We should not be touching GET/RELEASE_ARRAYS part of locking on Java arrays unless we have something like JDK-8162461. As Kevin has mentioned there is no need to add extra RELEASE_ARRAYS call. We have initDecompressor() call coming during creation of JPEGImageLoader. The call for startDecompression() & decompressIndirect() is inside try_finally block. If we have any issues in startDecompression() & decompressIndirect() native calls I think we we will call disposeNative() code and it will take care of cleanup of resources. But in case of initDecompressor() we need to explicitly free the resources as done or call already present image_dispose() function. Also we can do cleanup of image_dispose() as it has compressor related code(Most probably taken from same source as it is present in JDK ImageIO). In case of ImageIO imageioJPEG.c we explicitly call image_dispose() if there are problems during initialisation(But looks like in some cases we might need to call free() as we are doing in case of JFX, it needs more analysis). 05-10-2018 Hi Kevin, both the below change are not expected. I shall remove these. +//#undef NEED_SHORT_EXTERNAL_NAMES +//typedef int boolean; 05-10-2018 I diffed the .00 and .01 webrevs and there is a change in one of the source files. Is this expected? --- a/modules/javafx.graphics/src/main/native-iio/libjpeg/jconfig.h +++ b/modules/javafx.graphics/src/main/native-iio/libjpeg/jconfig.h @@ -18,13 +18,14 @@ #undef NEED_BSD_STRINGS #undef NEED_SYS_TYPES_H #undef NEED_FAR_POINTERS /* we presume a 32-bit flat memory model */ -#undef NEED_SHORT_EXTERNAL_NAMES +//#undef NEED_SHORT_EXTERNAL_NAMES #undef INCOMPLETE_TYPES_BROKEN /* Define "boolean" as unsigned char, not int, per Windows custom */ #ifndef __RPCNDR_H__ /* don't conflict if rpcndr.h already read */ typedef unsigned char boolean; #endif +//typedef int boolean; #ifndef FALSE /* in case these macros already exist */ #define FALSE 0 /* values of boolean */ #endif 04-10-2018 I have two comments regarding the changes in jpegloader.c: 1. The following code In initDecompressor is not right: 1418 if (GET_ARRAYS(env, data, &src->next_input_byte) == NOT_OK) { 1419 RELEASE_ARRAYS(env, data, src->next_input_byte); If GET_ARRAYS fails then you should not call RELEASE_ARRAYS. It will either not have been pinned or it will already have been released. 2. I think the error return cases in decompressIndirect might also need to free cinfo->src, cinfo, and jerr (aka cinfo->err). Can you check this? 04-10-2018 Two follow-on bugs are registered from review discussion. JDK-8211362: Restrict export of libjpeg symbols from libjavafx_iio.so JDK-8211363: Use platform version of libjpeg on Linux 04-10-2018 The updated changes look like what I would expect, thanks. I still need time to do a review of the changes in jpegloader.c -- I spotted a couple possible issues in my first look, so need to do a more thorough review. 04-10-2018 Hi Kevin, Thanks for the review comments, Please take a look at the updated changes : http://cr.openjdk.java.net/~arapte/fx/8203884/webrev.01/ Changes: 1. Moved modules/javafx.graphics/src/main/legal/jpeg_v7.md to modules/javafx.graphics/src/main/legal/jpeg_fx.md 2. Moved libjpeg7 to libjpeg folder using hg mv. 3. Some of the other library do not use version in folder name as libffi, libwebrtc, libxslt. So changed folder name to libjpeg as you suggested. 4. Corrected white spaces. 03-10-2018 I haven't fully reviewed the changes in jpegloader.c, so this is a partial review. That will take a bit more time. 1. Testing looks good. I did a build / test on all three platforms. 2. You need to rename (via hg mv) modules/javafx.graphics/src/main/legal/jpeg_v7.md to modules/javafx.graphics/src/main/legal/jpeg_v9.md and update the version in that file; another option is to rename it to jpeg_fx.md (but not just jpeg else it will conflict with one in the jdk). 3. Make sure you generate the patch such that the files that are moved are recorded as moved via hg. To verify, look at "hg diff --git" 4. Have you considered putting the libjeg files in a version independent path (e.g., native-iio/libjpeg rather than native-iio/libjpeg9c) so that they don't have to move next time? I don't know which is better, but this would avoid having to worry about #3 next time. 5. There are many white space problems that need to be fixed... 02-10-2018