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
