[OpenJDK 2D-Dev] Review Request for JDK-8139183 : drawImage misses background's alpha channel

Tue Mar 8 00:30:26 UTC 2016

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 at 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 at 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 at 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
>>>