Simone Giannecchini a écrit :
> Martin writes:
> "I didn't ported yet the changes done in the getImageLayout(...)
> method, because the coverage branch contains some changes that I do
> not yet understand. Why the tile size computation were replaced by a
> single tile covering the whole image? "
>
> I need more details in order to understand this question but, anyhow,
> I do not see where this replacement is.
Looked again and realized that my statement was wrong (I confused
"tileSize.getWidth()" with
"image.getWidth()" - sorry for that, it must have been again a little bit late
when I was on that
code...).
I will also add the two following lines, as in the coverage branch:
layout.setTileGridXOffset(image.getMinX());
layout.setTileGridYOffset(image.getMinY());
However, I wonder why:
if ((image.getNumXTiles() == 1 && image.getNumYTiles() == 1)
where extended as:
if ((image.getNumXTiles() == 1 && image.getNumYTiles() == 1)
|| (image.getTileWidth() == image.getWidth()
&& image.getTileHeight() <= image.getHeight()))
If the tile width is equals to the image width, doesn't it imply that numXTiles
== 1? Also, why
using the == operation for tile width and <= for the tile height? Do you means
"if the image is
tiled only vertically, lets retile it"?
> I am looking at the getImageLayout method right now. The one thing I
> did not like about the old method for calculating the tile size was
> that (beyond being a bit obscure or me :-) ) it aimed to computing a
> tile size such that the tile grid would have overlapped the image
> bound in order to avoid having tiles crossing the image bounds and
> being therefore partially empty.
> Well this approach is good for small coverage but it can be a problem
> with medium and big coverages since you end up having too many tiles
> (the so called tile explosions) which is a big performance problem for
> some operations.
>
> Please refere to this discussion thread I opened in the JAI ml.
> http://forums.java.net/jive/thread.jspa?messageID=112181
Added a great deal of comments (using some of yours comments on the JAI thread
:) ) in order to
explain what the method is doing. Will be committed in the next 24 hours.
The ImageUtilities class has a MIN_TILE_SIZE constant, which is currently
defined to 128. The tile
size proposed by ImageUtilities should never be smaller than 128. This constant
exists specifically
in order to prevent having too many small tiles. We can revisit this constant
value if you wish, or
pass it as a parameter to the function. We may also promote this function to a
more public place
(all classes in org.geotools.resources... should not be used outside Getools)
if you believe that it
worth to appears in a public API with its improved documentation.
Basically, this method tries to find a tile size wich is:
* a dividor of the image size
* in the range [MIN_TILE_SIZE .. MAX_TILE_SIZE]
* is the closest value of the default tile size (usually 512).
If no dividor is found, then this method try to find the tile size that
maximise the remainder of
"image size" / "tile size". In other words, the tile size that left as few
empty pixels as possible.
This tile size must always be constrained in the interval [MIN_TILE_SIZE ..
MAX_TILE_SIZE]. If no
suitable tile size is found, the function returns the plain default tile size
(usually 512).
> Of course, we can discuss about the changes I made and improve them.
> As you can notice that method is not what I would call definitive.
After the addition of layout.setTileGridX/YOffset and my above question about
the "if" statement,
the only remaining difference I see right now is that the old version set the
tile width and height
independently. I mean, if it was able to suggest a tile width but unable to
suggest a tile height,
then only the "tile width" image layout was set and the "tile height" image
layout was left
unspecified (i.e. the decision left to JAI implementation). The new code setup
tile width and tile
height together in all cases (when the image was not already tiled). I'm not
sure which approach is
the most appropriate...
[...about tileImage...]
> 1> Let's look t the following chain
>
> Read ----> Scale by N ----> crop -->visualize
>
> [...snip...]
>
> Once the operation reach the sink (the display device) pixels are
> pulled through the chain, tile by tile. Since the raster we have is
> untiled we will scale it all even if just need a small portion of it
> because it is made up by only one tile!
Not necessarly. The above paragraph is true if no ImageLayout hint were
provided to the "Scale"
operation. In my understanding of JAI working (and also what seems to happen
with the few JAI
operators I wrote), we just need to provide the ImageLayout hint to the "Scale"
operation. The image
resulting from the "Scale" operation will be tiled, and only the requested tile
will be computed. We
don't need a separated "Format" operation for that.
> If we do this:
> Read ----> tiling --> Scale by N ----> crop -->visualize
> Things are way better since after the tiling we will use and retain
> only the tiles we need even if the underlying image is untiled.
In my understanding, the ImageLayout hint can be applied to (almost) any kind
of operation. Using a
separated "Format" operation for that consumes some space in the tile cache for
an operation
(tiling) that could have been done directly by the "Scale" operation. The main
constraint is that we
must invoke ImageUtilities.getRenderingHints(image) before every JAI operations
in order to make
sure that we get tiled images when the source image was untiled. Invoking the
"Format" method free
the user from invoking ImageUtilities.getRenderingHints(image) before every
operation (JAI will
inherit the tile layout from the source image). It may have value if the users
were going to work
often directly with JAI operations. But if the users work rather with
GridCoverage, the
GridCoverageProcessor can ensure that ImageUtilities.getRenderingHints(image)
before to delegate to
the JAI operator.
> 2> What if we want to add native tiling to a coverage?
>
> We need to tile it first in memory and then we can write it on disk in
> order to have it naively tiled next time, hence we need the tileImage
> image method.
Yes. Right now this is the only usage for "tileImage" I can see (unless we
don't want to invoke
ImageUtilities.getRenderingHints(image) before each JAI operation). But this
operation could be
performed right into the grid coverage writer I think?
> "Other methods were refactored in a new class:
> org.geotools.image.ImageWorker. Some methods were close to identical,
> so I tried to avoided duplication (hopping that they still have the
> correct behavior)...."
>
> Martin plz go easy with renaming because you are going to kiil all the
> code I have which relies on those methods!!! :-)
We can keep those methods in ImageUtilities for the transition phase. But I
would like to refactor
them in such a way than one method do one specific thing, and leverage common
code in a single
place, if possible. For example "rescale2Bytes" used its own code for computing
tile size instead of
relying on 'getRenderingHints(image)', without obvious difference.
'direct2ComponentColorModel' and
'reformatColorModel2ComponentColorModel' seem to do almost the same thing (the
later being a little
bit more generic). It was also possible to factor out some duplicated code in
private methods.
However, of course I can't be sure that this refactoring will not break any
code. I will do my best
to keep the damage low...
> I have to admit that I was a bit surprised that you started the merge
> basically without asking anybody, because me, Andrea Aime and Jesse
> Eichar were fine tuning the last minor aspects but it is fine we will
> do that afterwards.
Just looked on the mail archive
(http://sourceforge.net/mailarchive/forum.php?forum_id=3008) but
couldn't found a more recent email than June 1, so I can't really point to an
email there. But I
posted some emails about that:
"Automated code formatting + coverage branch merge" on June 20
"IP Review Completed for AI Module" on June 21
"Coverage branch merge: feedbacks" on June 22
"Coverage branch merge: in progress" on June 22
Andrea, Jody and you were in CC of the first one. Maybe I should have posted
those messages with a
longer delay. But I just took 10 days vacation specifically for that (and
JSR-275) and may be able
to work on the coverage branch merge only this week...
> In case you have some questions plz send some emails on the devel list
> so that I can try to explain what I did and, in case, we can change
> it!
Sure. I had hesitation about what would be the most appropriate place (the
mailing of GEOT-873,
since I didn't wanted to anoy the whole mailing list with my questions). But I
can post them to the
mailing list if people prefer.
Thanks for your input,
Martin.
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
Geotools-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
