[OpenJDK 2D-Dev] Review Request for bug (JDK-8080287): The image of BufferedImage.TYPE_INT_ARGB and BufferedImage.TYPE_INT_ARGB_PRE is blank

[prasanta sadhukhan]

Hi Jim,All

I have modified the code following your comments. Please find the 
modified webrev:
http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.02/
Could you please review this?

Regards
Prasanta
On 6/23/2015 2:03 AM, Jim Graham wrote:
> In reading through this I notice that at line 349 the filter code can 
> permanently change the nature of the RescaleOp.  It should use a local 
> copy of the length field if it is going to reinterpret it on the fly 
> like that.
>
> Also, at line 348 - shouldn't it do something if length > numBands, 
> but the source does not have alpha?  Or is this due to the fragile 
> tests for "length == numBands+1" below which use that to determine if 
> some special processing should be done for alpha? The handling of the 
> relationship between length, numBands and alpha in general seems to be 
> very fragile and full of assumptions about how the setup code managed 
> those values without any code comments saying "we leave these 
> variables in this relationship to indicate that we need to do X, Y, or 
> Z".  I'd prefer various conditions to be reflected in appropriate 
> boolean variables that are reasonably descriptive rather than 
> undocumented assumptions about how length relates to numBands.  Also, 
> numBands should probably be renamed to numColorBands to avoid any 
> issues such as what Andrew noted below.
>
> The test at line 397 seems backwards.  Shouldn't it be "(numBands+1 == 
> length)"?  What exactly is it testing?  It looks like it is deciding 
> if it should eliminate alpha from the scaling loops since "length==1" 
> is defined to only modify the colors, but then it subsets the color 
> bands which means if you supply only one scale then you scale all but 
> the alpha band and also all but one of the color bands.  Or am I 
> misreading something?
>
>             ...jim
>
> On 6/22/2015 2:58 AM, Andrew Brygin wrote:
>> Hello Prasanta,
>>
>>   I have couple comments regarding the fix.
>>
>> *  lines 408 - 420 and lines 438 - 444.
>>      Here you are obtaining the source and destination rasters for all
>> bands (colors + alpha).
>>      However, it is already done on the lines 391 and 392.
>>       Could you please clarify a purpose of this change?
>>
>> * line 399: here 'numBands' represents number of color bands in the
>> source image (see line 329).
>>     So, the last color band is excluded from processing (for example, in
>> RGB image you get raster
>>     that contain only R and G bands).
>>
>> * you have created a manual test. Probably an automated test is a bit 
>> more
>>     convenient option here.
>>     Also, there seems to be no need for a jpg image for this test. A
>> source image
>>     with color strips is much more useful.
>>
>> Thanks,
>> Andrew
>>
>> On 6/22/2015 12:36 PM, prasanta sadhukhan wrote:
>>> Hi ,
>>>
>>> Please review a fix for this issue:
>>> It was found that RescaleOp on image with different alpha cannot
>>> render the image as there is a particular flaw in RescaleOp
>>> implementation whereby the source alpha channel is never
>>> transferred to the destination if the rescale op is performed in java
>>> (or is never populated, if source image has no alpha channel),
>>> resulting in fully transparent destination image.
>>> Fix is to make sure the unscaled source alpha is transferred to
>>> destination alpha channel.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8080287
>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.00/
>>>
>>> Regards
>>> Prasanta
>>