[gdal-dev] Notes on gdalbuildvrt.cpp

[Even Rouault]

Le jeudi 21 août 2014 19:32:34, Nicu Tofan a écrit :
> Assuming that VRTBuilder class inside gdalbuildvrt.cpp has been created to
> be reusable, 

Not particularly, but your remarks are interesting and beneficial to 
gdalbuildvrt itself.

> I have some comments/questions that, depending on your input,
> may end up in pull requests.
> 
>    - *VRTBuilder::VRTBuilder, line 300*
> 
> Instead of assigning the pointer create a copy to be consistent with the
> rest of the code. The memory is released twice, once at 1558 with
> 
> CPLFree( panBandList );
> 
> and once at 347 with
> 
> delete[] panBandList;

True, I could actually reproduce the issue with valgrind and passing the -b 
option of gdalbuildvrt.
But there are code paths in VRTBuilder::AnalyseRaster() that allocate 
panBandList in some circumstances. So the right fix would be in the VRTBuilder 
constructor to do a copy of the passed panBandList if not NULL

> 
>    - *VRTBuilder::pasDatasetProperties line 237*
> 
> At line 1092 AnalyseRaster is called with a pointer inside
> pasDatasetProperties
> 
> if (AnalyseRaster( hDS, dsFileName, &pasDatasetProperties[i] ))
> 
> However the array may be re-allocated at line 416; if the reallocation is
> not in place the psDatasetProperties pointer will be invalid.
> 
> Maybe passing an index instead of a pointer can solve the problem.

I'm not sure there is an issue there. AnalyseRaster() will possibly relocate 
pasDatasetProperties, but as this is a member variable of the class, the next 
call of line 1092 will have the new address. Do you have a practical case 
where there would be an issue ?

> 
>    - *line 513-520*
> 
> What would it take to support rotated, positive NS resolutions?

Generally this is a sign of non georeferenced images. Perhaps some adaptations 
should be made if there are assumptions in the rest of the code about the sign 
of the y resolution

> 
>    - *line 543*
> 
> Assumes that there is at least one band; The code below does not make that
> assumption and checks for that condition; GDALGetBlockSize() does not
> handle a NULL pointer.

True. Should be moved after line 576 likely.

> 
>    -
> *line 825, 920 *
> 
> Why the GDALProxyPoolDatasetH? Why not opening it like a regular data set?
> When creating VRT files in C++ code this is the path to be taken?

If you are creating a VRT with more than 1024 sources, you would run out of 
file descriptors on Linux. GDALProxyPoolDatasetH is a trick to avoid having too 
many "real" datasets opened at the same time. If you don't have that 
constraint, you could use the real datasets, but this is needed in the general 
case of gdalbuildvrt.

Even

-- 
Spatialys - Geospatial professional services
http://www.spatialys.com