[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Review Request for bug (JDK-8080287): The image of BufferedImage.TYPE_INT_ARGB 
From:       Jim Graham <james.graham () oracle ! com>
Date:       2015-06-30 22:38:16
Message-ID: 55931A58.7010709 () oracle ! com
[Download RAW message or body]

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
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]