[mkgmap-dev] [Patch] Error in BoundaryUtil

From GerdP gpetermann_muenchen at hotmail.com on Mon Mar 12 21:20:50 GMT 2012

Hi WanMil,

the length field will only help when we want to allow an old mkgmap version
to 
read a newer bnd format. I don't think that we need that.
Any (incompatible) change in the record layout should increase the record
id,
so that a newer version can decide what fields are available.

Gerd


WanMil wrote
> 
> 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@
>>  > To: mkgmap-dev at .org
>>  > Subject: Re: [mkgmap-dev] [Patch] Error in BoundaryUtil
>>  >
>>  > > Hi WanMil,
>>  > >
>>  > >
>>  > >
>>  > > > Date: Sun, 11 Mar 2012 20:08:54 +0100
>>  > > > From: wmgcnfg@
>>  > > > To: mkgmap-dev at .org
>>  > > > 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@
>>  > > > > > To: mkgmap-dev at .org
>>  > > > > > 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 .org
>>  > > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>  > > > > >
>>  > > > > > _______________________________________________
>>  > > > > > mkgmap-dev mailing list
>>  > > > > > mkgmap-dev at .org
>>  > > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>  > > > >
>>  > > > >
>>  > > > > _______________________________________________
>>  > > > > mkgmap-dev mailing list
>>  > > > > mkgmap-dev at .org
>>  > > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>  > > >
>>  > > > _______________________________________________
>>  > > > mkgmap-dev mailing list
>>  > > > mkgmap-dev at .org
>>  > > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>  > >
>>  > >
>>  > > _______________________________________________
>>  > > mkgmap-dev mailing list
>>  > > mkgmap-dev at .org
>>  > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>  >
>>  > _______________________________________________
>>  > mkgmap-dev mailing list
>>  > mkgmap-dev at .org
>>  > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
>>
>>
>> _______________________________________________
>> mkgmap-dev mailing list
>> mkgmap-dev at .org
>> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> 
> _______________________________________________
> mkgmap-dev mailing list
> mkgmap-dev at .org
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> 


--
View this message in context: http://gis.19327.n5.nabble.com/Patch-Error-in-BoundaryUtil-tp5554370p5559192.html
Sent from the Mkgmap Development mailing list archive at Nabble.com.

---

• Previous message: [mkgmap-dev] [Patch] Error in BoundaryUtil
• Next message: [mkgmap-dev] [Patch] Error in BoundaryUtil
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

---

More information about the mkgmap-dev mailing list