Hi Jay,
The code changes look fine. There are a few issues with the use of
VolatileImage in the test case, though.
First, contentsLost is meant to be executed after an operation, not
before. The class comments give an example of using it both during
rendering to the VI and also during copying from the VI. You are doing
both in one operation here, but in both cases, the contentsLost test is
after the operation has completed. To be clear, you should fill the VI,
draw the test image into it, snapshot it, and then after all of that is
done, test contentsLost and loop back around.
Second, the test for IMAGE_INCOMPATIBLE and the way you recurse could
end up in an infinite loop in theory. In practice, these issues are
rare and only happen if we have a change in screen configurations during
the use of a VI. The validate() call is meant to check a VI that has
been sitting around from a previous operation to see if it can be used
again for a new operation on a new repaint cycle. If you look at the
sample code in the VI documentation the response to an INCOMPATIBLE
image is to just create a new one and go on, assuming that it will be OK
because it would be nearly impossible for an image to become
incompatible between creating it and immediately using it. As such, I'd
just delete the test here as it won't add any value. If it were to be
left in, it should only be done a couple of times before giving up and
throwing a test error as something must be wrong in the system if it
happens more than once (actually something is likely wrong in the system
even if it happens just once immediately after creating a brand new
image). So, either delete the check or throw a test failure (even
though it isn't a failure for the bug at hand, it indicates something
wrong in the 2D code).
Finally, AlphaComposite.CLEAR ignores the color so there is no need to
also set a transparent color to clear the VI. If it wasn't being
cleared by using AlphaComposite.CLEAR, then there is another bug going
on there. An alternative is to use AlphaComposite.SRC and the
transparent color, but just using CLEAR and setting no color at all
should be fine...
...jim
On 3/4/16 3:05 AM, Jayathirth D V wrote:
Hi Jim,
I have updated the code to remove indentation problem and added VolatileImage
also for testing.
Please find the updated webrev for review :
http://cr.openjdk.java.net/~jdv/8139183/webrev.03/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Friday, March 04, 2016 5:21 AM
To: Jayathirth D V; Sergey Bylokhov
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8139183 : drawImage misses
background's alpha channel
Hi Jay,
The new code looks good except for one minor indentation problem on the argument list of
the method after you added the private keyword. Either back out the keyword or re-indent
the argument list continuation line to match either the "(" of the method or
the standard indent.
And, VolatileImage.getSnapshot() should help with testing VolatileImage
results...
...jim
On 3/3/2016 6:13 AM, Jayathirth D V wrote:
Hi,
Thanks for pointing to logical mistake I made Jim. I have made changes to use
bg color transparency to determine what type of buffered image should be used
in makeBufferedImage().
Also based on Sergey's suggestions included other source types like ARGB_PRE in
test case and made makeBufferedImage() API private since it is only used in
DrawImage.java. But I was not able to include VolatileImage as source type as I
didn't find a way to automate the test to verify alpha value after drawImage(),
since there is no API to get pixel value from VolatileImage. While debugging I
was able to make sure that in case VolatileImage also proper type is selected
before makeBufferedImage() is called.
Please review updated webrev:
http://cr.openjdk.java.net/~jdv/8139183/webrev.02/
Thanks,
Jay
-----Original Message-----
From: Jim Graham
Sent: Thursday, March 03, 2016 5:19 AM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race; Prasanta
Sadhukhan
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8139183 :
drawImage misses background's alpha channel
The logic here has mixed up the opacities and what needs to be done about them.
If the source image is opaque, then this is not a BG operation at all because
the bg color would not show through an opaque image, so checking the srcData is
both wrong and should be a NOP here. If we get into that block with a srcData
that is opaque then something has gone wrong somewhere else. In particular,
isBgOp should have returned false in that case.
The transparency that matters is when the bg color has transparency and that is
not what is being tested here. The test for xRGB and ARGB should be using the
bg color...
...jim
On 3/2/16 5:17 AM, Jayathirth D V wrote:
Hi,
I have updated the changes to select proper Buffer Image type based
on source transparency and not just using ARGB directly.
Please find the updated webrev for review:
http://cr.openjdk.java.net/~jdv/8139183/webrev.01/
Thanks,
Jay
*From:* Jayathirth D V
*Sent:* Wednesday, March 02, 2016 5:02 PM
*To:* 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan
*Subject:* Review Request for JDK-8139183 : drawImage misses
background's alpha channel
Hi,
_Please review the following fix in JDK9:_
__
Bug : https://bugs.openjdk.java.net/browse/JDK-8139183
Webrev : http://cr.openjdk.java.net/~jdv/8139183/webrev.00/
Issue : When we scale any buffered image using drawImage() API which
takes scale coordinates we are losing alpha channel in background color.
Root cause : We are creating opaque temporary image when we have
background color and scale is happening in renderImageXform() API of
DrawImage.java. By making it opaque we are losing translucency.
Solution : Instead of creating opaque RGB temporary image use ARGB
temporary image to maintain translucency of image.
Thanks,
Jay
