s57, memory leak, problems with converting polygon, problems when applying updates, s57tables missing one s57-3.1 feature

[vaernes@…]

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.

[vaernes@…]

s57tables

[vaernes@…]

s57reader

[vaernes@…]

a57classregistrar

[vaernes@…]

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

[vaernes@…]

(From update of attachment 234)
added missing object class

[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.

[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. 

[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".

[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.

[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()"

[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

[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

[warmerdam]

ah yes, I see the problem.

I am patching this in CVS. 

Thanks

[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 

[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.

[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

[warmerdam]

Rune,

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

Thanks!

[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. 

[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

[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.