[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for bug (JDK-8080287): The image of BufferedImage.TYPE_INT_ARGB
From: Phil Race <philip.race () oracle ! com>
Date: 2015-08-04 23:29:59
Message-ID: 55C14AF7.8090601 () oracle ! com
[Download RAW message or body]
+1.
-phil.
On 08/04/2015 01:53 PM, Jim Graham wrote:
> That is true. I usually don't inspect the class I'm testing to see if
> a test is needed since the implementation could change. Instead I
> determine what conditions I want to verify and test those conditions
> whether or not I "know" that the code would be doing the right thing
> there.
>
> It's OK to go now either way...
>
> ...jim
>
> On 8/3/15 4:30 PM, prasanta sadhukhan wrote:
>> 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
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic