Hi Prasanta, As far as I know imageDataProvider_UnholdJavaPixels() is only invoked for images with ‘custom’ type. We read the data directly from Java memory for such images. At the same time for other image types we use Get/ReleasePrimitiveArrayCritical() and copy the data from Java to native memory. I guess that’s the main reason why we don’t set isdo->pixels to null inside imageDataProvider_UnholdJavaPixels().
Also I updated the test based on your suggestions. Please find the new version here: http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/ Thanks, Dmitry > On 23 Jan 2017, at 09:17, Prasanta Sadhukhan <[email protected]> > wrote: > > Hi Dmitry, > Any particular reason why in imageDataProvider_UnholdJavaPixels(), we are not > doing > isdo->pixels = null; which we do in unholdJavaPixels() > > since in holdJavaPixels() , we are doing > 967 else if (isdo->pixels == NULL) > 968 { > 969 isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, > isdo->array)); > 970 } > > Also, in the test, I guess there's no point catching Exception and rethrowing > RuntimeException since we are not doing any processing in catch block. We can > just add throws Exception in main() without this catch block and have > try-finally. > Also, I guess we do not follow adding "author" in the new test anymore. > > Regards > Prasanta > On 1/22/2017 12:35 AM, Dmitry Markov wrote: >> Hi Phil, >> >> I agree ‘othervm’ is not necessary here. That is ‘copy-paste’ error. >> Also I updated the part related to the file deletion based on your >> suggestion. >> Please find new webrev here: >> http://cr.openjdk.java.net/~dmarkov/8163889/webrev.02/ >> <http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.02/> >> >> Thanks, >> Dmitry >>> On 21 Jan 2017, at 00:24, Philip Race <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> Hi Dmitry, >>> > 29 * @run main/othervm PrintCrashTest >>> why othervm ? >>> >>> I don't think that is strictly necessary just because you are using >>> deleteOnExit. >>> And FWIW I think the test could "more promptly" delete the file anyway >>> after print returns. >>> >>> -phil. >>> >>> >>> On 1/20/17, 9:36 AM, Dmitry Markov wrote: >>>> >>>> Hi Phil, Prasanta, >>>> >>>> I have updated the fix as you suggested, (i.e. added the regression test). >>>> The new webrev is located at >>>> http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/ >>>> <http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.01/> >>>> Could you review the new version, please? >>>> >>>> Thanks, >>>> Dmitry >>>>> On 20 Jan 2017, at 19:50, Phil Race <[email protected]> >>>>> <mailto:[email protected]> wrote: >>>>> >>>>> I haven't looked at the fix (yet) but I definitely agree that a manual >>>>> regression test >>>>> for this is better than none. What else should we do ? Just not test >>>>> printing ? >>>>> >>>>> In my view which I've expressed to SQE for a really long time, if you >>>>> aren't testing with >>>>> printers installed you aren't testing the whole platform. Whilst it may >>>>> be convenient >>>>> that tests (silently) don't complain when there are no printers, it is a >>>>> slippery slope .. >>>>> >>>>> -phil. >>>>> >>>>> On 01/20/2017 04:04 AM, Prasanta Sadhukhan wrote: >>>>>> It is possible to create manual regression test for this problem. Also >>>>>> the test will require some additional set up steps such as printer >>>>>> installation and so on. It seems to me that is overhead for person who >>>>>> runs it. However if you insist on test creation, I will add it. >> >
