[OpenJDK 2D-Dev] Review Request for bug (JDK-8080287): The image of BufferedImage.TYPE_INT_ARGB and BufferedImage.TYPE_INT_ARGB_PRE is blank
Jim Graham
james.graham at oracle.com
Mon Aug 3 20:08:38 UTC 2015
I agree with this. I think this applies for both test cases (src has
alpha and src does not).
Also, the println() statements should be removed before pushing it.
They don't harm anything, but we shouldn't have spurious random print
outs in our tests - they should only print anything on failure.
Another simplification is the test for alpha. When you get the rgb
value from bimg.getRGB() you can test it directly for alhpa without
having to make a color with:
if ((rgb >>> 24) != 255)
(I also think it would make sense to test for 255 rather than 0 since 0
is not the only value that would be wrong - it just happens to be the
value you currently get. Better to test for the right answer than "not
one of many wrong answers")
...jim
On 8/3/15 11:08 AM, Phil Race wrote:
> 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