[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
Mon Aug 3 23:30:47 UTC 2015
Hi Jim, Phil
Thanks for your comments.
Yes, I agree for src with no alpha channel, I needed to create
destination image with alpha channel and see if filtered image also has
alpha channel . I have modified this.
But, for src image with alpha channel, I guess I can still pass
destination image as "null" as the RescaleOp code creates a destination
image from src image in which case dest image will have alpha.
/if (dst == null) {//
// dst = createCompatibleDestImage(src, null);//
// dstCM = srcCM;//
// }//
/....
...
/public BufferedImage createCompatibleDestImage (BufferedImage src,//
//ColorModel destCM) {//
// BufferedImage image;//
// if (destCM == null) {//
// ColorModel cm = src.getColorModel();//
// image = new BufferedImage(cm,//
//src.getRaster().createCompatibleWritableRaster(),//
//cm.isAlphaPremultiplied(),//
// null);//
// }
/so I guess we do not need to pass a destination image with alpha
channel explicitly for source with alpha channel sub testcase.
Jim, is there anything I am missing?
Here's the modified webrev:
http://cr.openjdk.java.net/~psadhukhan/8080287/webrev.06/
Regards
Prasanta
On 8/4/2015 1:38 AM, Jim Graham wrote:
> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20150804/5417bad9/attachment.html>
More information about the 2d-dev
mailing list