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
