Hello, Anthony. Please see the updated version here: http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.03/
I've added the autorelease to the imageRep. Checked that no memory leaks in the new methods are present. With best regards. Petr. On 07.03.2014, at 15:15, Anthony Petrov <[email protected]> wrote: > Hi Petr, > > src/macosx/native/sun/awt/CImage.m: >> 452 NSBitmapImageRep* imageRep = CImage_CreateImageRep(env, buffer, >> width, height); >> 453 if (imageRep) { >> 454 NSData *tiffImage = [imageRep TIFFRepresentation]; >> 455 jsize tiffSize = (jsize)[tiffImage length]; >> 456 result = (*env)->NewByteArray(env, tiffSize); >> 457 CHECK_NULL_RETURN(result, nil); >> 458 jbyte *tiffData = (jbyte >> *)(*env)->GetPrimitiveArrayCritical(env, result, 0); >> 459 CHECK_NULL_RETURN(tiffData, nil); >> 460 [tiffImage getBytes:tiffData]; >> 461 (*env)->ReleasePrimitiveArrayCritical(env, result, tiffData, 0); >> 462 [imageRep release]; > > I see that we're managing the imageRep manually. So it is not a part of the > auto-release pool created with the JNF_COCOA_ENTER macro. Which means that > the return statements at lines 457 and 459 will lead to a memory leak. I > suggest to use @finally to ensure that the imageRep gets released (this is > what the JNF_COCOA_EXIT does, too). > > > -- > best regards, > Anthony > > On 3/7/2014 10:59 AM, Petr Pchelko wrote: >> Hello, >> >> Please review the updated version of the fix: >> http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.02/ >> >> I've moved the image handling code from DataTransferer to CImage. This let >> us reuse the existing code that creates an NSBitmapImageRep from the int >> array and let us get rid of some JNI upcalls. >> >> Tested with all SQE datatransfer tests and out reg tests. No new failures. >> >> With best regards. Petr. >> >> On 06.03.2014, at 19:38, Petr Pchelko <[email protected]> wrote: >> >>> Hello, Sergey. >>> >>> To retrieve the image from native we are using NSDeviceRGBColorSpace (see >>> CImage.m), so we need to use the same color space to put the image into >>> native. If i revert this line no calibrated space the test would fail >>> because the image we get from native is different from the one we put. >>> >>> I’ve just got an idea. CImage API is able to put the Java Image into >>> native, so we could probably reuse it here. Let me investigate this, I’ll >>> write you back when I succeed or fail. >>> >>> With best regards. Petr. >>> >>> 06 марта 2014 г., в 7:22 после полудня, Sergey Bylokhov >>> <[email protected]> написал(а): >>> >>>> Hi, Petr. >>>> How NSCalibratedRGBColorSpace change affects the fix? >>>> >>>> On 3/6/14 5:30 PM, Petr Pchelko wrote: >>>>> Hello again. >>>>> >>>>> I’ve updated the fix. The new version is here: >>>>> http://cr.openjdk.java.net/~pchelko/9/8030710/webrev.01/ >>>>> >>>>> 1. I’ve modified the test so it now doesn’t need .html file, also >>>>> copyright was added. >>>>> 2. I’ve replaced the public.jpeg mime with a predefined kUTTypeJPEG >>>>> (which is the same). >>>>> No such constant in Cocoa, only in Carbon. >>>>> >>>>> With best regards. Petr. >>>>> >>>>> 06 марта 2014 г., в 2:56 после полудня, Petr Pchelko >>>>> <[email protected]> написал(а): >>>>> >>>>>> Hello, Sergey. >>>>>> >>>>>>> I guess JFIF is not a typo in CDataTransferer? >>>>>> Nop. http://en.wikipedia.org/wiki/JPEG_File_Interchange_Format >>>>>> Also please see the macosx flavormap.properties type. We call a native >>>>>> as a JFIF and it’s left as is because >>>>>> it’s called like this on other platforms. >>>>>> >>>>>>> Do we really need ImageTransferTest.html in the test? >>>>>> The test is for interprocess DnD, so we run the first process as an >>>>>> applet from jtreg. >>>>>> The applet then starts a second processes main(). I’ll check if I could >>>>>> use 2 main() in one file and if it’s fine for jtreg. >>>>>> >>>>>>> Also copyright header is missing. >>>>>> Thank you. I’ll update the fix shortly. >>>>>> >>>>>> With best regards. Petr. >>>>>> >>>>>> 06 марта 2014 г., в 2:49 после полудня, Sergey Bylokhov >>>>>> <[email protected]> написал(а): >>>>>> >>>>>>> Hi, Petr. >>>>>>> I guess JFIF is not a typo in CDataTransferer? Do we really need >>>>>>> ImageTransferTest.html in the test? >>>>>>> Also copyright header is missing. >>>>>>> >>>>>>> On 3/5/14 8:23 PM, Petr Pchelko wrote: >>>>>>>> Hello, AWT Team. >>>>>>>> >>>>>>>> Please review the fix for the issue: >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8030710 >>>>>>>> The fix is available at: >>>>>>>> http://cr.openjdk.java.net/~pchelko/9/8030710/webrev/ >>>>>>>> >>>>>>>> The problem was that on Mac we thought that re support PNG and JPEG >>>>>>>> image formats, but in reality we did not have mappings for these >>>>>>>> formats. I've added the missing mappings. >>>>>>>> >>>>>>>> The test is being open sourced. The fix was checked with out tests and >>>>>>>> with native-Java DnD. >>>>>>>> >>>>>>>> With best regards. Petr. >>>>>>> >>>>>>> -- >>>>>>> Best regards, Sergey. >>>>>>> >>>> >>>> >>>> -- >>>> Best regards, Sergey. >>>> >>> >>
