[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 prasanta.sadhukhan at oracle.com
Thu Jul 30 21:57:22 UTC 2015

Hi Jim,

On 7/30/2015 5:46 AM, Jim Graham wrote:
> Hi Prasanta,
> That looks good now.  Can 480,482 be combined into just "return dst"?
Yes, done.
> Also, Andrew mentioned in the first review pass that he would rather 
> see an automated test.  I agree with that assessment.  There is no 
> need to display a result to ask a user to verify something we can do 
> by examining the pixel values in the result...
Modified webrev with automated test is below:

> ...jim
> On 7/29/15 11:21 AM, prasanta sadhukhan wrote:
>> Hi Jim,
>> Thanks for your comments. My observations inline..
>> On 7/29/2015 9:22 AM, Jim Graham wrote:
>>> Hi Prasant,
>>> Can you check what the ImagingLib call at line 401 will do if we have
>>> the case that scaleConst != length?  We used to modify "this.length"
>>> at line 350, but now we no longer do that (because it was wrong), but
>>> the code in ImagingLib may have depended on that.
>> As far I see, ImagingLib does not do anything for RescaleOp so this
>> change will not have any effect. It has functionality for LookupOp,
>> AffineTransformOp and ConvolveOp and just fall back to Java for
>> RescaleOp to do the needful.
>>> Similarly, at line 445 we call this.filter(raster, raster) which
>>> expects this.length to be an appropriate value - and if it could see
>>> the value of scaleConst that we've calculated, that value would be
>>> correct, but it is expecting that value to have been passed through
>>> via the this.length field.  We must not modify this.length so we need
>>> a way to say "filter the rasters, but using these other constants that
>>> I've calculated, not the ones you see in the fields.  That is what I
>>> was referring to when I said that the implementation part of
>>> filter(Raster, Raster) should be split out into a method that takes
>>> parameters from one of the 2 front end methods.  We need to do that
>>> here so that filter(Image, Image) can tell it to convert only
>>> scaleConst number of bands without having to modify the length field.
>> Have split filter(Raster, Raster) into implementation method for
>> filer(Image,Image) to call with scaleConst.
>>> Lines 378 and 383 - origDst is used to convert the rescaled raster
>>> back to the color space of the original destination.  But, at line 378
>>> we've already replaced the original destination with a newly created
>>> one so we are not saving the original destination, we are just making
>>> a second reference to the dst.  Later, at line 478, we convert dst to
>>> dst which is a NOP.
>> I guess origDst is replaced by newly created one to avoid a scenario
>> whereby filter(Image, Image dst=null) is called so needToConvert will be
>> false and so line 478 will not be executed and thereafter if we return
>> origDst at end, we will be returning null. I tried to circumvent this by
>> storing origDst at start but I again store the rescaled dst back to
>> origDst (in case if origDst is null ) which is returned at the end.
>>> Lines 408-440 - we can put a test for "if (!scaleAlpha)" around the
>>> entire block rather than testing it separately in each of the
>>> src/dest.hasAlpha() blocks.
>>> Line 447 - we should not copy the alphas if scaleAlpha is true, should
>>> we?  The scaling done in the raster should have already copied and
>>> scaled them and you'd be replacing them with unscaled copies of the
>>> original...
>> Have taken care of this.
>> Please find the modified webrev here:
>> http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.03/
>> Please let me know if this is ok.
>> Regards
>> Prasanta
>>> ...jim
>>> On 6/30/15 3:38 PM, Jim Graham wrote:
>>>> The backup code if ImagingLib does not do the work is to forward the
>>>> request to the filter(Raster, Raster) method which again tests length
>>>> and requires it to be == src.getNumBands().  An instance to this Op is
>>>> also passed to ImagingLib in that method and it's not clear what will
>>>> happen if length doesn't match when that happens either.  It may 
>>>> simply
>>>> ignore the operation and we end up in the backup code, or it may 
>>>> get an
>>>> exception.  In any case, since the length field was not modified by
>>>> filter(Bimg, Bimg), we've changed the conditions in the downstream 
>>>> code.
>>>> What I'd generally like to see in cases like this is that the public
>>>> methods do validation and then they pass only validated information to
>>>> helper routines which are clearly private.  For things like "in this
>>>> case we don't want to operate on all of the bands of the image", it
>>>> would be nice if the helper routines could be told to work on 
>>>> explicitly
>>>> defined bands rather than having to compute child rasters with subset
>>>> bands.  Doing all of that would take quite a bit of work, but 
>>>> perhaps we
>>>> can at least do the following:
>>>> - provide a filterRaster() helper method that filter(Raster x 2) 
>>>> and the
>>>> backup case for filter(Bimg x 2) both call after validation
>>>> - the filterRaster() helper method would take a length parameter and
>>>> ignore the field value
>>>> - ImagingLib interfaces may have to be upgraded as well to take a 
>>>> length
>>>> parameter, I haven't looked at ImagingLib yet to see how it would be
>>>> affected by these changes
>>>> That much should be fairly easy to arrange, and in doing that we may
>>>> discover that it would be easy to have ImagingLib take a list of 
>>>> subset
>>>> bands which might help us avoid doing all of the createChildRaster 
>>>> calls
>>>> in filter(Bimg)...
>>>>              ...jim
>>>> On 6/28/2015 11:44 PM, prasanta sadhukhan wrote:
>>>>> Hi Jim,
>>>>> I was following the RescaleOp spec where it states
>>>>> /The number of sets of scaling constants may be one, in which case 
>>>>> the
>>>>> same constants are applied to all color (but not alpha) components/
>>>>> which is taken care by
>>>>> /if (numSrcColorComp == scaleConst || //*scaleConst == 1*//) {//
>>>>> //*scaleAlpha = false*;//
>>>>> //        }//
>>>>> //Otherwise, the number of sets of scaling constants may equal the
>>>>> number of Source color components, in which case no rescaling of the
>>>>> alpha component (if present) is performed/
>>>>> /if (*numSrcColorComp == scaleConst* || //scaleConst == 1//) {//
>>>>> //*scaleAlpha = false*;//
>>>>> //        }//
>>>>> ////If neither of these cases apply, the number of sets of scaling
>>>>> constants must equal the number of Source color components plus alpha
>>>>> components, in which case all color and alpha components are rescaled
>>>>> //For Rasters, rescaling operates on bands. The number of sets of
>>>>> scaling constants may be one, in which case the same constants are
>>>>> applied to all bands, or it must equal the number of Source Raster
>>>>> bands. /
>>>>> which is taken care by above check.
>>>>> Earlier, we had
>>>>> int[] bands = new int[numBands-1];
>>>>> which omitted the last color bands (which I could not find in the 
>>>>> spec
>>>>> if that is what we should do). So, I changed to
>>>>> int[] bands = new int[numSrcColorComp];
>>>>> Regards
>>>>> Prasanta
>>>>> On 6/25/2015 3:17 AM, Jim Graham wrote:
>>>>>> Hi Prasanta,
>>>>>> I just realized that this method uses filter(Raster, Raster) to 
>>>>>> do its
>>>>>> work and so some of these changes may affect how it communicates 
>>>>>> with
>>>>>> that method.  This will take some time to look through all of the
>>>>>> interactions.  In particular, the code that modified the length
>>>>>> parameter, while still wrong in the long run, may have had the side
>>>>>> effect of making some of the operations succeed by making sure the
>>>>>> right preconditions existed for the raster case...
>>>>>>             ...jim
>>>>>> On 6/23/15 11:30 PM, prasanta sadhukhan wrote:
>>>>>>> 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

More information about the 2d-dev mailing list