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

[Jim Graham]

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
>