[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
Sat Aug 1 00:32:42 UTC 2015
Hi Jim,
Thanks for your comments. In fact, Phil and I was discussing the same
thing. I have modified the same.
Please find the modified webrev:
http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.05/
Regards
Prasanta
On 8/1/2015 4:54 AM, Jim Graham wrote:
> Hi Prasanta,
>
> Check your new code for trailing white space - I found quite a bit of
> extra spaces at the ends of lines.
>
> I just realized that I never really looked closely at the loop that
> copies alpha. It doesn't appear to do anything. In particular:
>
> 460 int alpha = 0xff;
>
> 463 int color = dst.getRGB(cx, cy);
> 464
> 465 int mc = (alpha << 24) | 0x00ffffff;
>
> mc is -1 here. You've essentially used a complicated expression to
> accumulate a bunch of on bits into mc.
>
> 466 int newcolor = color & mc;
>
> because mc is -1, newcolor is now the same as color because (v & -1)
> == v.
>
> 467 dst.setRGB(cx, cy, newcolor);
>
> this is a NOP because it is just storing the original color back into
> the dest.
>
> I'm guessing that you are trying to change the alpha of all colors to
> 0xff? Then this is the proper loop:
>
> int alpha = 0xff << 24;
> for (int cy=0; cy < dst.getHeight(); cy++) {
> for (int cx=0; cx < dst.getWidth(); cx++) {
> int color = dst.getRGB(cx, cy);
> dst.setRGB(cx, cy, color | (0xff << 24));
> }
> }
>
> Also, in the test - please don't use Robot or create a Window or
> subclass Frame. Just create an image, call RescaleOp on it, and test
> the pixel in the resulting image. That is all you need. No Frame, no
> MediaTracker, no Robot, no show(), no golden images - none of that.
>
> The test should be about 5 lines long:
>
> - create Bimg
> - fill it with green
> - create RescaleOp
> - filter Bimg with RescaleOp
> - test result.getRGB(x, y) for some pixel in the image
>
> ...jim
>
> On 7/30/15 2:57 PM, prasanta sadhukhan wrote:
>> 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