On 6/26/06, Martin Desruisseaux <[EMAIL PROTECTED]> wrote: > 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"? >
That is done because there are many formats that use stripes as an alternative to tiles, an example is tiff. A stripe can be a performance black hole, I have seen stripes as large as 20.000 columns x 8 rows. If you just want to see a chunk of 512x512 think about how much data you have to load!!! > > > 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. > What you said is true, but I go this way for a reason that is related to my concerns with the way we computed the tile size and the problem of tile explosion. I think it is a bit too much to recompute the tile size at each operation for various reasons. Once you inject a raster into a rendering chain in JAI if that raster is tiled its tile size will be retained as much as possible through the chain, which means that at each node the tile grid of the output raster will be similar of the one of the source raster. It is different for example if the output image is smaller than the source tile size (in this case output tile size will be equal to the output raster dimension) or if you crop or translated in which case the tilegrid offset might change, or again if you scale in which case the tile grid offset are again recomputed (not sure though :-) ). For these reasons it is unneeded to recompute the tile size implicitly at each step but it is better to compute it at once and then retain it. I add also that it would be good to add the possibility to control the cache at each step of a coverage op but this is something that we need to do to all the operation in gridprocessing package (it is a capability that right now is missing, if I am right :-) ). Beside all this, there is an additional reason to avoid having very different tile sizes between processing chains, that is TILE RECYCLING introduce as of JAI 1.1.2rc (see http://java.sun.com/products/java-media/jai/README.html). Tile recycling allow us to recycle discarded tiles from the cache, hence we do not have to rebuilt them completely. TO fully exploit this capability, which by the way is really cool, we need to have tiles similar to each other as much as possible. I am aware that we cannot control sample model and color model but we can try to keep at least the size as more uniform as possible between different operations (this is the reason why there is a default tile size in JAI). Anyway your approach of using the image layout is a very cool alternative that I, mistakenly, have not taken much into account. > > > 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? > Yeah, correct :-). > > > > "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... > > Do not worry do as much damage as you want, you usually make things better so I think it is worth some damage. > > > 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... > DO not worry man, you are doing much more than expected. Taking a vacation for working on another project, we can really complain for anything... :-) > > > 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. > Thanks man, Simone. -- °°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°° Simone Giannecchini Software Engineer Freelance Consultant http://simboss.wordpress.com/ °°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°°° 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
