[mkgmap-dev] [Patch] Error in BoundaryUtil

Mon Mar 12 21:55:27 GMT 2012

Gerd,

I have commited your patch plus the header length implementation. The 
int for the record layout should give the format of the records not the 
header. So I don't see a reason to change the record layout id if I just 
want to add a field to the header and therefore I need the header length.

WanMil

> Hi Gerd,
>
> the length field is required to be able to add more fields to the header
> in the future. Example (please don't nail me with the given length of
> the fields - it's just an example):
>
> Header:
> Format type: String: BND
> Timestamp: long
> | Header length: int: 8+4+10+4 = 26 bytes
> | Record format: String: RAW (8 bytes)
> | Record layout: int: 0x01 (4 bytes)
> | mkgmap release: String: 2248 (10 bytes)
> | Additional id: int: 0x45 (4 bytes)
> ... Records ...
>
> Now think about three different reader versions all trying to read that
> file:
>
> Reader "old" does not know about the new additional id. But it can read
> until the mkgmap release and can skip the 4 bytes fo the additional id
> (due to the known header length).
>
> Reader "current" expects exactly all fields until the additional id. It
> reads 26 bytes and finishes.
>
> Reader "future" expects an additional field "Java version". But after
> reading the additional id it can detect that the header does not contain
> this new field "Java version". Therefore it can assume a default value
> for that.
>
> If you don't use the header length you won't have the chance to add
> anything to the header. (You've had the same problem with the legacy
> format...)
>
> WanMil
>
>
>> Hi WanMil,
>>
>> I don't see a need for the length field in the header, and I don't see
>> how I could use it
>> (I found no way to calculate how many bytes were removed from a stream)
>>
>> So I've coded this:
>> Format type: String: BND
>> Timestamp: long
>> | Record format: String: (RAW, QUADTREE)
>> | Record layout: int
>> | mkgmap release: String
>> ... Records ...
>>
>> Attached is the patch that implements this.
>>
>> Gerd
>>
>> > Date: Sun, 11 Mar 2012 21:02:56 +0100
>> > From: wmgcnfg at web.de
>> > To: mkgmap-dev at lists.mkgmap.org.uk
>> > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>> >
>> > > Hi WanMil,
>> > >
>> > >
>> > >
>> > > > Date: Sun, 11 Mar 2012 20:08:54 +0100
>> > > > From: wmgcnfg at web.de
>> > > > To: mkgmap-dev at lists.mkgmap.org.uk
>> > > > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>> > > >
>> > > > i Gerd,
>> > > >
>> > > > can you please explain in short what's the difference between the
>> three
>> > > > formats (my thinking):
>> > > > * Legacy (original - int coords)
>> > > > * Raw (float coords)
>> > > > * Quadtree (admin level areas merged to the quadtree format)
>> > >
>> > > yes, correct
>> > >
>> > > >
>> > > > loadQuadTree (Quadtree format?) => loadQuadTreeFromStream (Quadtree
>> > > > format?) => readStreamRawFormat (Raw format?) => readArea (Legacy
>> format)
>> > > >
>> > > > At least it is irritating to me if there is a method
>> readStreamRawFormat
>> > > > which reads raw and/or legacy format.
>> > >
>> > > I agree. I'll change that so that we have two methods
>> > > readStreamLegacyFormat and readStreamRawFormat.
>> >
>> > Great! Maybe it makes also sense to put the detection of the format
>> into
>> > one method? In the moment there are a lot of rel.endsWith(...)
>> checks in
>> > the code.
>> >
>> > >
>> > > >
>> > > > I would also propose a change to the header of the new raw and
>> quadtree
>> > > > format. In the legacy format the first String in the file gives the
>> > > > release of mkgmap. You added a "_raw" or "_quadtree" (?) to mark
>> the new
>> > > > formats. This is unclean but required to support the legacy
>> format. But
>> > > > legacy format support can be dropped in the future (I think at
>> least in
>> > > > the end of 2012). So to be open for future other changes it
>> would be
>> > > > good to have a kind of format identification. This could be added
>> after
>> > > > the creation time. Having this format identification future
>> changes to
>> > > > the format could be easily and much cleaner added.
>> > >
>> > > Yes, my problem with the legacy format was that it did not contain
>> such
>> > > a format identifier.
>> > > I've worked a long time with programs on IBMs mainframe system
>> (OS/390,
>> > > z/OS).
>> > > They typically use an eye catcher to identify the record type and a
>> > > numeric field that
>> > > identifies the record layout version. In some programs, they have
>> also a
>> > > length field.
>> > > Old programs can read newer structures because the newer fields are
>> only
>> > > added
>> > > at the end of a structure, and the length field allows to skip them.
>> > > I think this is a good idea, but we probably don't have to support
>> all
>> > > kinds of
>> > > versions in programs and files.
>> > >
>> > > So, here is my suggestion:
>> > > instead of adding _raw or _quadtree to the 1st string I replace it
>> with BND.
>> > > Legacy format is identified because it doesn't contain this string.
>> > > A new String field fileFormat after the timestamp identifies the file
>> > > format,
>> > > either RAW or QUADTREE.
>> > > One more int field recId identifies the record layouts used (0 for
>> this
>> > > first version).
>> > > Whenever a new format is needed, we change fileFormat, if only a new
>> > > field is needed in e.g. the AREA section, we increase recId.
>> > > All methods that read the file verify that they are able to
>> process the
>> > > recId
>> > > or they stop with an FormatException .
>> > > OK?
>> >
>> > Sounds good. But I propose two additional things:
>> > 1. a header size or id (otherwise we will have the same problem
>> again to
>> > add an additional information to the header)
>> > 2. Keep an mkgmap release identifier.
>> >
>> > So the new format would look like:
>> > Format type: String: BND
>> > Header size: int
>> > | Record format: String: (RAW, QUADTREE)
>> > | Record layout: int
>> > | Timestamp: long
>> > | mkgmap release: String
>> > ... Records ...
>> >
>> > WanMil
>> >
>> >
>> > >
>> > > Gerd
>> > >
>> > > >
>> > > > WanMil
>> > > >
>> > > > > Hi WanMil,
>> > > > >
>> > > > > hmm, we have two errors here. I can confirm that my patch
>> doesn't solve
>> > > > > all problems,
>> > > > > it just allows again to read the old *.bnd format and the new
>> quadtree
>> > > > > format, but mkgmap
>> > > > > fails to read the intermediate format produced by the
>> preparers 1st
>> > > pass.
>> > > > >
>> > > > > > Date: Sun, 11 Mar 2012 18:25:21 +0100
>> > > > > > From: wmgcnfg at web.de
>> > > > > > To: mkgmap-dev at lists.mkgmap.org.uk
>> > > > > > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>> > > > > >
>> > > > > > Hi Gerd,
>> > > > > >
>> > > > > > without your patch preparing boundaries works.
>> > > > >
>> > > > > No, it doesn't. It stops (crashes) without any error message when
>> > > trying
>> > > > > to read the *.bnd
>> > > > > files produced in the 1st pass. You can verify this by looking
>> at the
>> > > > > header of
>> > > > > the new *.bnd files, they have the suffix _raw, not _quadtree.
>> > > > >
>> > > > > The new patch fixes this, but not the missing error detection.
>> > > > >
>> > > > > I think we need a try/catch block somewhere, either in
>> Preparer or
>> > > > > BoundaryPreparer.
>> > > > > Will you take care of this?
>> > > > >
>> > > > > Gerd
>> > > > >
>> > > > >
>> > > > > > Using your patch I get the following exception.
>> > > > > >
>> > > > > > java.util.concurrent.ExecutionException:
>> > > > > > java.lang.IllegalArgumentException: Ill
>> > > > > > egal Capacity: -572471432
>> > > > > > at java.util.concurrent.FutureTask$Sync.innerGet(Unknown
>> Source)
>> > > > > > at java.util.concurrent.FutureTask.get(Unknown Source)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.main.Preparer.runPreparer(Preparer.java:92)
>> > > > > > at uk.me.parabola.mkgmap.main.Main.endOptions(Main.java:349)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.CommandArgsReader.readArgs(CommandArgsReader.ja
>> > > > > > va:126)
>> > > > > > at uk.me.parabola.mkgmap.main.Main.main(Main.java:114)
>> > > > > > Caused by: java.lang.IllegalArgumentException: Illegal
>> Capacity:
>> > > > > -572471432
>> > > > > > at java.util.ArrayList.<init>(Unknown Source)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.readStreamRawF
>> > > > > > ormat(BoundaryUtil.java:282)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTreeFr
>> > > > > > omStream(BoundaryUtil.java:459)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTree(B
>> > > > > > oundaryUtil.java:145)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryUtil.loadQuadTree(B
>> > > > > > oundaryUtil.java:120)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryPreparer$QuadTreeWo
>> > > > > > rker.call(BoundaryPreparer.java:236)
>> > > > > > at
>> > > > > >
>> uk.me.parabola.mkgmap.reader.osm.boundary.BoundaryPreparer$QuadTreeWo
>> > > > > > rker.call(BoundaryPreparer.java:221)
>> > > > > > at java.util.concurrent.FutureTask$Sync.innerRun(Unknown
>> Source)
>> > > > > > at java.util.concurrent.FutureTask.run(Unknown Source)
>> > > > > > at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown
>> > > > > > Source)
>> > > > > > at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown
>> > > > > > Source)
>> > > > > > at java.lang.Thread.run(Unknown Source)
>> > > > > >
>> > > > > > My mkgmap call is:
>> > > > > > java -jar mkgmap.jar --max-jobs=4
>> > > > > > --createboundsfile=africa-boundaries.osm.gz
>> > > --bounds=bounds/africa *.osm
>> > > > > >
>> > > > > > WanMil
>> > > > > >
>> > > > > > > Hi WanMil,
>> > > > > > >
>> > > > > > > sorry, one of my last patches for the performance branch
>> corrupted
>> > > > > > > BoundaryUtil.
>> > > > > > > It was no longer able to read the legacy *.bnd format (also
>> created
>> > > > > in the
>> > > > > > > preparer) :-(
>> > > > > > >
>> > > > > > > Attached is the patch.
>> > > > > > >
>> > > > > > > Gerd
>> > > > > > >
>> > > > > > >
>> > > http://gis.19327.n5.nabble.com/file/n5554370/BoundaryUtil.java.patch
>> > > > > > > BoundaryUtil.java.patch
>> > > > > > >
>> > > > > > > --
>> > > > > > > View this message in context:
>> > > > >
>> > >
>> http://gis.19327.n5.nabble.com/Patch-Error-in-BoundaryUtil-tp5554370p5554370.html
>>
>> > > > > > > Sent from the Mkgmap Development mailing list archive at
>> > > Nabble.com.
>> > > > > > > _______________________________________________
>> > > > > > > mkgmap-dev mailing list
>> > > > > > > mkgmap-dev at lists.mkgmap.org.uk
>> > > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>> > > > > >
>> > > > > > _______________________________________________
>> > > > > > mkgmap-dev mailing list
>> > > > > > mkgmap-dev at lists.mkgmap.org.uk
>> > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>> > > > >
>> > > > >
>> > > > > _______________________________________________
>> > > > > mkgmap-dev mailing list
>> > > > > mkgmap-dev at lists.mkgmap.org.uk
>> > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>> > > >
>> > > > _______________________________________________
>> > > > mkgmap-dev mailing list
>> > > > mkgmap-dev at lists.mkgmap.org.uk
>> > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>> > >
>> > >
>> > > _______________________________________________
>> > > mkgmap-dev mailing list
>> > > mkgmap-dev at lists.mkgmap.org.uk
>> > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>> >
>> > _______________________________________________
>> > mkgmap-dev mailing list
>> > mkgmap-dev at lists.mkgmap.org.uk
>> > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>
>>
>> _______________________________________________
>> mkgmap-dev mailing list
>> mkgmap-dev at lists.mkgmap.org.uk
>> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>