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

Tue Mar 8 08:56:56 UTC 2016

Hi Jim,

Thanks for the review. I have made changes in test case based on input provided.
1) Added contentsLost() after we drawImage() and getSnapshot() to verify the content and if it is lost loop again.
2) Removed recursive creation of VolatileImage based IMAGE_INCOMPATIBLE property.
3) Removed setColor() with transparent color for AlphaComposite.Clear as it is not needed.

Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8139183/webrev.04/ 

Thanks,
Jay

-----Original Message-----
From: Jim Graham 
Sent: Tuesday, March 08, 2016 6:00 AM
To: Jayathirth D V
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 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
>>>