[OpenJDK 2D-Dev] Review Request for bug (JDK-8080287): The image of BufferedImage.TYPE_INT_ARGB and BufferedImage.TYPE_INT_ARGB_PRE is blank
Phil Race
philip.race at oracle.com
Mon Aug 3 18:08:39 UTC 2015
65 // Test with source image without alpha channel
66 System.out.println("Test with source image without alpha channel");
67
68 bimg = new BufferedImage(w, h, BufferedImage.TYPE_INT_RGB);
69 g2d = bimg.createGraphics();
70 g2d.setColor(Color.GREEN);
71 g2d.fillRect(0, 0, w, h);
72
73
74 res = new RescaleOp(scaleFactor, offset, null);
75 bimg1 = res.filter(bimg, null);
In this case don't you need to supply a destination image which has an
alpha channel ?
Else I don't think you are testing alpha at all here ..
bimg1 = new BufferedImage(w, h, BufferedImage.TYPE_INT_ARGB);
bimg1 = res.filter(bimg, bimg1);
-phil.
On 07/31/2015 05:32 PM, prasanta sadhukhan wrote:
> 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