Hi Dmitri,
On 1/23/2017 11:52 PM, Dmitry Markov wrote:
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.
I understand that but my question was coming from the fact that, if we
are going to print more than 1 "custom" image, then isdo->pixels will
point to the same "direct buffer address" (which we obtained for the
first custom image) since isdo->pixels will not be NULL (so line 969
will not be executed), whereas ideally, isdo->pixels should obtain the
data again. Am I misinterpreting something?
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/
<http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.03/>
test looks good.
Regards
Prasanta
Thanks,
Dmitry
On 23 Jan 2017, at 09:17, Prasanta Sadhukhan
<[email protected]
<mailto:[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 athttp://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/
Could you review the new version, please?
Thanks,
Dmitry
On 20 Jan 2017, at 19:50, Phil Race<[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.
