[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:
http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.04/
Regards
Prasanta
> ...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