Opened 19 years ago
Last modified 19 years ago
#840 closed defect (fixed)
s57, memory leak, problems with converting polygon, problems when applying updates, s57tables missing one s57-3.1 feature
Reported by: | Owned by: | warmerdam | |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | OGR_SF | Version: | unspecified |
Severity: | normal | Keywords: | |
Cc: |
Description
s57classregistrar: -There is a memory leak with pachAttrType, pachAttrClass and panAttrIndex s57table: -missing object class 160 Tidal Stream s57reader: -AssembleAreaGeometry should not delete poLine and continue when there is no poline. -FindAndApplyUpdates fail when the update extension number is heigher then 9 and the function does not support "Primar" generated cds.
Attachments (3)
Change History (20)
by , 19 years ago
Attachment: | s57tables.h added |
---|
comment:1 by , 19 years ago
(From update of attachment 235) The code i have introduced in FindAndApplyUpdates does not use CPL but std
comment:3 by , 19 years ago
Rune, You don't mention which version of GDAL you were working against. I have regenerated the s57tables.h from the files in gdal/data, and committed it to CVS. It seems it was not regenerated when the last set of updates (more than just the tidal stream) was made. You might want to grab the latest from: http://www.gdal.org/srctree/ogr/ogrsf_frmts/s57/s57tables.h I have applied the suggested leak fixes in s57classregistrar, and also fixed a leak of some names i papszAttrNames and papszAttrAcronym.
comment:4 by , 19 years ago
Rune, Can you point me to a dataset that have a polygon line with only end points? One that demonstrates the problem with returning NULL in AssembleAreaGeometry() when FetchLine fails? I think you are right, but I would be alot more comfortable applying the change if I could see the problem and fix in action. Also, can I get a dataset in the Primar organization? I don't really follow you Primar related code, and I will want to reimplement it.
comment:5 by , 19 years ago
Frank, I'm using gdal-1.2.6. The the dataset which has the polygon problem is "Great Britain" the filname is GB202000. The file structure on the CD is as follows: ENC_ROOT/"<COUNTRYCODE>"/"<CHARTNAME>"/"<MAJORVERSION/RVES>"/"<FILEVERSION>"/"<F ILE>" ex. ENC_ROOT/NO/NO11111/16/0/NO11111.000 ENC_ROOT/NO/NO11111/16/1/NO11111.001 ENC_ROOT/NO/NO11111/16/2/NO11111.002 http://www.primar-stavanger.org/ is the "Primar".
comment:6 by , 19 years ago
Frank, In s57reader I also added an extra check "if( poTarget != NULL )" in the "while ( nNextFEIndex < oFE_Index.GetCount() )" Without this check the s57reader does not produce a complete dataset with all metainformation.
comment:7 by , 19 years ago
(In reply to comment #9) > Frank, > In s57reader I also added an extra check "if( poTarget != NULL )" in the "while > ( nNextFEIndex < oFE_Index.GetCount() )" Without this check the s57reader does > not produce a complete dataset with all metainformation. Sorry forgott the methode "ReadNextFeature()"
comment:8 by , 19 years ago
Frank, I can mail you the dataset which fails when creating a polygon, but then you must promise me that it's for you eyes only and that you only will use it to test the polygon problem. -- Rune
comment:9 by , 19 years ago
Hey, I have tried your leak fixed with the s57classregistrar.cpp. The destuctor fail in the for-loop for papszAttrNames when i = 24999. My temporary fix for this is : papszAttrNames = (char **) CPLCalloc(sizeof(char *),MAX_ATTRIBUTES); papszAttrAcronym = (char **) CPLCalloc(sizeof(char *),MAX_ATTRIBUTES); pachAttrType = (char *) CPLCalloc(sizeof(char),MAX_ATTRIBUTES); pachAttrClass = (char *) CPLCalloc(sizeof(char),MAX_ATTRIBUTES); panAttrIndex = (int *) CPLCalloc(sizeof(int),MAX_ATTRIBUTES); -- Rune
comment:11 by , 19 years ago
Howdy, Did you receive the dataset ? Have you tested it ? I have tried to convert about 10 cd's with s57-data, almost all datasets was converted. (The cd's are product by different primar's) Chould I just send a bug report with a problem description or try to make a pacth for you ? -- rune
comment:12 by , 19 years ago
Rune, I have the example data, but I have not yet tried to do anything with it. Nor have I worked on adding the primar directory support. I'm afraid this remaining portion dropped down in my priority queue to "inactive" for the time being. You are in good working condition, right? So there is no time pressure. I may not get to it till late June after a variety of travel and other crunch items are out of the way.
comment:13 by , 19 years ago
Hey, There is no time pressure. I will countinue to fix problems I find, I'll send them to you as fast as possible. -- rune
comment:14 by , 19 years ago
Rune, I have applied the patch for handling linestrings with no vertices other than the end points after verifying it. Thanks!
comment:15 by , 19 years ago
Rune, I have applied the findupdates change. I haven't verified it works for primar data, but it doesn't seem to do any harm.
comment:16 by , 19 years ago
(In reply to comment #18) > Rune, > I have applied the findupdates change. I haven't verified it works for > primar data, but it doesn't seem to do any harm. Sorry I thought you where going to rewrite the findandupdate code I introduced, since it doesn't follow the rest of the implementation. You must changed the length of the char buf[] arrays with -1. if( 1 <= iUpdate && iUpdate < 10 ) { char buf[2] => buf[1]; ... } else if( 10 <= iUpdate && iUpdate < 100 ) { char buf[3] => buf[2] ; ... } else if( 100 <= iUpdate && iUpdate < 1000 ) { char buf[4] => buf[3]; ... } -- rune
comment:17 by , 19 years ago
Rune, In the following code I had already increased buf[1] to buf[2] as sprintf() writes not only the single digit but also a trailing \0 character. Without the increase memory checking tools were reporting a stack overflow (though on most systems it would have been harmless). if( 1 <= iUpdate && iUpdate < 10 ) { char buf[2]; sprintf( buf, "%i", iUpdate ); extension.append("00"); extension.append(buf); dirname.append(buf); } Were you thinking there was some reason the buf[]s needed to be shrunk again? Also, looking at the code, I think something like would replace all 3 case: char buf[4]; sprintf( buf, "%03d", iUpdate ); extension.append(buf); dirname.append(buf); The %03d format strings means a 3digit number with leading zeros inserted as needed. However, I don't have time to test such a change now, so I will leave it alone.
Note:
See TracTickets
for help on using tickets.
s57tables