[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review request for 8163889: [macosx] Can't print from browser on Mac OS X
From: Prasanta Sadhukhan <prasanta.sadhukhan () oracle ! com>
Date: 2017-01-24 6:52:46
Message-ID: c20b310b-94e4-1d5b-14b7-541b79af8d39 () oracle ! com
[Download RAW message or body]
ok. +1
Regards
Prasanta
On 1/24/2017 12:04 PM, dmitry markov wrote:
> Hi Prasanta,
>
> Please find my answer inline.
>
> Thanks,
> Dmitry
> On 24/01/2017 08:29, Prasanta Sadhukhan wrote:
> >
> > 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?
> Actually we initialize isdo structure based on data from Java layer
> anytime when an image is printed, see LockImage() function. So
> isdo->pixels will be correctly initialized inside holdJavaPixels() for
> every "custom" image. I am sorry, but the situation you described
> above is not possible here.
> > > 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
> > > > <prasanta.sadhukhan@oracle.com
> > > > <mailto:prasanta.sadhukhan@oracle.com>> 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 <philip.race@oracle.com
> > > > > > <mailto:philip.race@oracle.com>> 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<philip.race@oracle.com> 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.
> > > > >
> > > >
> > >
> >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>ok. +1<br>
</p>
Regards<br>
Prasanta<br>
<div class="moz-cite-prefix">On 1/24/2017 12:04 PM, dmitry markov
wrote:<br>
</div>
<blockquote cite="mid:5886F57D.4090601@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Prasanta,<br>
<br>
Please find my answer inline.<br>
<br>
Thanks,<br>
Dmitry<br>
<div class="moz-cite-prefix">On 24/01/2017 08:29, Prasanta
Sadhukhan wrote:<br>
</div>
<blockquote
cite="mid:68ccebf0-3619-7796-a791-05ebec257fd2@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<p>Hi Dmitri,<br>
</p>
<br>
<div class="moz-cite-prefix">On 1/23/2017 11:52 PM, Dmitry
Markov wrote:<br>
</div>
<blockquote
cite="mid:E0926A23-85D4-4667-8FAB-9E5CA15B2638@oracle.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">Hi Prasanta,</span></div>
<div style="margin: 0px; line-height: normal; min-height:
14px;" class=""><span style="font-kerning: none" class=""></span><br
class="">
</div>
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">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. <br>
</span></div>
</blockquote>
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?<br>
</blockquote>
Actually we initialize isdo structure based on data from Java
layer anytime when an image is printed, see LockImage() function.
So isdo->pixels will be correctly initialized inside
holdJavaPixels() for every "custom" image. I am sorry, but the
situation you described above is not possible here. <br>
<blockquote
cite="mid:68ccebf0-3619-7796-a791-05ebec257fd2@oracle.com"
type="cite">
<blockquote
cite="mid:E0926A23-85D4-4667-8FAB-9E5CA15B2638@oracle.com"
type="cite">
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">At the same time for
other image types we use
Get/ReleasePrimitiveArrayCritical() and copy the data from
Java to native memory.</span></div>
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">I guess that's the
main reason why we don't set isdo->pixels to null
inside imageDataProvider_UnholdJavaPixels().</span></div>
<div style="margin: 0px; line-height: normal; min-height:
14px;" class=""><span style="font-kerning: none" class=""></span><br
class="">
</div>
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">Also I updated the
test based on your suggestions. Please find the new
version here: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.03/"
class="">http://cr.openjdk.java.net/~dmarkov/8163889/webrev.03/</a></span></div>
</blockquote>
test looks good.<br>
<br>
Regards<br>
Prasanta<br>
<blockquote
cite="mid:E0926A23-85D4-4667-8FAB-9E5CA15B2638@oracle.com"
type="cite">
<div style="margin: 0px; line-height: normal; min-height:
14px;" class=""><span style="font-kerning: none" class=""></span><br
class="">
</div>
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">Thanks,</span></div>
<div style="margin: 0px; line-height: normal;" class=""><span
style="font-kerning: none" class="">Dmitry </span></div>
<div style="margin: 0px; line-height: normal; min-height:
14px;" class=""><span style="font-kerning: none" class=""></span><br
class="">
</div>
<div>
<blockquote type="cite" class="">
<div class="">On 23 Jan 2017, at 09:17, Prasanta Sadhukhan
<<a moz-do-not-send="true"
href="mailto:prasanta.sadhukhan@oracle.com" \
class="">prasanta.sadhukhan@oracle.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000" class="">
<p class="">Hi Dmitry,<br class="">
</p>
<p class="">Any particular reason why in
imageDataProvider_UnholdJavaPixels(), we are not
doing <br class="">
</p>
<p class="">isdo->pixels = null; which we do in
unholdJavaPixels() </p>
since in holdJavaPixels() , we are doing <br class="">
<pre class="">967 else if (isdo->pixels == NULL)
968 {
969 isdo->pixels = (Pixel8bit*)((*env)->GetDirectBufferAddress(env, \
isdo->array)); 970 }
</pre>
<div class="moz-cite-prefix">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.<br class="">
Also, I guess we do not follow adding "author" in
the new test anymore. <br class="">
<br class="">
Regards<br class="">
Prasanta<br class="">
On 1/22/2017 12:35 AM, Dmitry Markov wrote:<br
class="">
</div>
<blockquote
cite="mid:C43DC296-7D01-40F9-93A3-9BF69FED6654@oracle.com"
type="cite" class="">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8" class="">
Hi Phil,
<div class=""><br class="">
</div>
<div class="">I agree ‘othervm' is not necessary
here. That is ‘copy-paste' error. </div>
<div class="">Also I updated the part related to the
file deletion based on your suggestion.</div>
<div class="">Please find new webrev here: <a
moz-do-not-send="true"
\
href="http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.02/"
\
class="">http://cr.openjdk.java.net/~dmarkov/8163889/webrev.02/</a></div> <div \
class=""><br class=""> </div>
<div class="">Thanks,</div>
<div class="">Dmitry <br class="">
<div class="">
<blockquote type="cite" class="">
<div class="">On 21 Jan 2017, at 00:24, Philip
Race <<a moz-do-not-send="true"
href="mailto:philip.race@oracle.com"
class="">philip.race@oracle.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type" class="">
<div bgcolor="#FFFFFF" text="#000000"
class=""> Hi Dmitry,<br class="">
<meta http-equiv="content-type"
content="text/html; charset=utf-8"
class="">
> 29 * @run main/othervm PrintCrashTest<br
class="">
<pre class="">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.
</pre>
<br class="">
<br class="">
On 1/20/17, 9:36 AM, Dmitry Markov wrote:
<blockquote
\
cite="mid:C1BB8E44-79B5-40EB-BFAC-CAC1EA4F35B0@oracle.com" type="cite" class="">
<pre class="" wrap="">Hi Phil, Prasanta,
I have updated the fix as you suggested, (i.e. added the regression test). The new \
webrev is located at <a moz-do-not-send="true" class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/%7Edmarkov/8163889/webrev.01/">http://cr.openjdk.java.net/~dmarkov/8163889/webrev.01/</a>
Could you review the new version, please?
Thanks,
Dmitry
</pre>
<blockquote type="cite" class="">
<pre class="" wrap="">On 20 Jan 2017, at 19:50, \
Phil Race <a moz-do-not-send="true" class="moz-txt-link-rfc2396E" \
href="mailto:philip.race@oracle.com"><philip.race@oracle.com></a> 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:
</pre>
<blockquote type="cite" class="">
<pre class="" wrap="">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. </pre>
</blockquote>
</blockquote>
</blockquote>
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic