Opened 16 years ago

Last modified 15 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: vaernes@… 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)

s57tables.h (38.3 KB) - added by vaernes@… 16 years ago.
s57tables
s57reader.cpp (91.9 KB) - added by vaernes@… 16 years ago.
s57reader
s57classregistrar.cpp (18.8 KB) - added by vaernes@… 16 years ago.
a57classregistrar

Download all attachments as: .zip

Change History (20)

Changed 16 years ago by vaernes@…

Attachment: s57tables.h added

s57tables

Changed 16 years ago by vaernes@…

Attachment: s57reader.cpp added

s57reader

Changed 16 years ago by vaernes@…

Attachment: s57classregistrar.cpp added

a57classregistrar

comment:1 Changed 16 years ago by vaernes@…

(From update of attachment 235)
The code i have introduced in FindAndApplyUpdates does not use CPL but std

comment:2 Changed 16 years ago by vaernes@…

(From update of attachment 234)
added missing object class

comment:3 Changed 16 years ago by warmerdam

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 Changed 16 years ago by warmerdam

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 Changed 16 years ago by vaernes@…

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 Changed 16 years ago by vaernes@…

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 Changed 16 years ago by vaernes@…

(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 Changed 16 years ago by vaernes@…

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 Changed 16 years ago by vaernes@…

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:10 Changed 16 years ago by warmerdam

ah yes, I see the problem.

I am patching this in CVS. 

Thanks

comment:11 Changed 16 years ago by vaernes@…

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 Changed 16 years ago by warmerdam

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 Changed 16 years ago by vaernes@…

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 Changed 15 years ago by warmerdam


Rune,

I have applied the patch for handling linestrings with no vertices other than
the end points after verifying it.

Thanks!

comment:15 Changed 15 years ago by warmerdam

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 Changed 15 years ago by vaernes@…

(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 Changed 15 years ago by warmerdam

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.