[gdal-dev] Notes on gdalbuildvrt.cpp

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[gdal-dev] Notes on gdalbuildvrt.cpp

Nicu Tofan
Assuming that VRTBuilder class inside gdalbuildvrt.cpp has been created to be reusable, 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;

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

  • line 513-520

What would it take to support rotated, positive NS resolutions?

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

  • 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?


Regards,

Nicu


_______________________________________________
gdal-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Notes on gdalbuildvrt.cpp

Even Rouault-2
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
_______________________________________________
gdal-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Notes on gdalbuildvrt.cpp

Nicu Tofan
Hello Even,

  • VRTBuilder::pasDatasetProperties line 237

DatasetProperty* psDatasetProperties

This is a variable allocated on the stack; on function entry it contains a pointer somewhere inside pasDatasetProperties; Say pasDatasetProperties is 0x7500000 and psDatasetProperties is
0x7500100. Now on line pasDatasetProperties the buffer at 0x7500000gets reallocated, possibly moving to, say, 0x90000000; psDatasetPropertiesshould now be 0x90000100 but it is still 0x7500100 because it is stored in a variable on the stack - namelly the argument psDatasetPropertie.

On line 457 psDatasetPropertie is used and is going to contain 0x750010. It may not trigger an access violation because the address will probably be inside the heap but it may generate some hard to find bugs that we all love and cherish.

As a reflection of mine, this may be the result of the naming convention, as psDataset and pasDataset look the same to our animal brains.


  • Generally this is a sign of non georeferenced images...

The bounding box becomes a bit more complicated in rotated images, too. I'm gonna look into it.


  • GDALProxyPoolDatasetH

Got it, thanks!


Regards,
Nick

PS Forgot to reply to the list, sorry.

_______________________________________________
gdal-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Notes on gdalbuildvrt.cpp

Even Rouault-2
Le jeudi 21 août 2014 21:13:15, Nicu Tofan a écrit :

> Hello Even,
>
>
>    - VRTBuilder::pasDatasetProperties line 237
>
> DatasetProperty* psDatasetProperties
>
> This is a variable allocated on the stack; on function entry it contains a
> pointer somewhere inside pasDatasetProperties; Say pasDatasetProperties is
> 0x7500000 and psDatasetProperties is
> 0x7500100. Now on line pasDatasetProperties the buffer at 0x7500000gets
> reallocated, possibly moving to, say, 0x90000000; psDatasetPropertiesshould
> now be 0x90000100 but it is still 0x7500100 because it is stored in a
> variable on the stack - namelly the argument psDatasetPropertie.
>
> On line 457 psDatasetPropertie is used and is going to contain 0x750010. It
> may not trigger an access violation because the address will probably be
> inside the heap but it may generate some hard to find bugs that we all love
> and cherish.

That shouldn't happen because of the return at line 453. But for clarity we
might have a dedicated function for the processing done between line 412 and
454 since it doesn't need psDatasetProperties at all.

>
> As a reflection of mine, this may be the result of the naming convention,
> as psDataset and pasDataset look the same to our animal brains.

Hungarian-style convention.

psXXXX is a Pointer to a single Structure
pasXXXXX is a Pointer to an Array of Structure

>
>
>
>    - Generally this is a sign of non georeferenced images...
>
> The bounding box becomes a bit more complicated in rotated images, too. I'm
> gonna look into it.

Dealing with non-zero values in gt[2] and gt[4] is clearly out of scope of
what a regular VRT can do (you need a warped VRT for that).
Well you could still produce a regular VRT (with a rotated geotransform) if
all the sources have the same rotation in their geotransform, but that's an
unlikely situation.

>
>
>
>    - GDALProxyPoolDatasetH
>
> Got it, thanks!
>
> Regards,
> Nick
>
> PS Forgot to reply to the list, sorry.

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
gdal-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/gdal-dev
Reply | Threaded
Open this post in threaded view
|

Re: Notes on gdalbuildvrt.cpp

Nicu Tofan
> That shouldn't happen because of the return at line 453....
Damn, I missed that. :)

> Hungarian-style convention....
Yes, I've read RFC8; just thinking out loud.




2014-08-21 22:21 GMT+03:00 Even Rouault <[hidden email]>:
Le jeudi 21 août 2014 21:13:15, Nicu Tofan a écrit :
> Hello Even,
>
>
>    - VRTBuilder::pasDatasetProperties line 237
>
> DatasetProperty* psDatasetProperties
>
> This is a variable allocated on the stack; on function entry it contains a
> pointer somewhere inside pasDatasetProperties; Say pasDatasetProperties is
> 0x7500000 and psDatasetProperties is
> 0x7500100. Now on line pasDatasetProperties the buffer at 0x7500000gets
> reallocated, possibly moving to, say, 0x90000000; psDatasetPropertiesshould
> now be 0x90000100 but it is still 0x7500100 because it is stored in a
> variable on the stack - namelly the argument psDatasetPropertie.
>
> On line 457 psDatasetPropertie is used and is going to contain 0x750010. It
> may not trigger an access violation because the address will probably be
> inside the heap but it may generate some hard to find bugs that we all love
> and cherish.

That shouldn't happen because of the return at line 453. But for clarity we
might have a dedicated function for the processing done between line 412 and
454 since it doesn't need psDatasetProperties at all.

>
> As a reflection of mine, this may be the result of the naming convention,
> as psDataset and pasDataset look the same to our animal brains.

Hungarian-style convention.

psXXXX is a Pointer to a single Structure
pasXXXXX is a Pointer to an Array of Structure

>
>
>
>    - Generally this is a sign of non georeferenced images...
>
> The bounding box becomes a bit more complicated in rotated images, too. I'm
> gonna look into it.

Dealing with non-zero values in gt[2] and gt[4] is clearly out of scope of
what a regular VRT can do (you need a warped VRT for that).
Well you could still produce a regular VRT (with a rotated geotransform) if
all the sources have the same rotation in their geotransform, but that's an
unlikely situation.

>
>
>
>    - GDALProxyPoolDatasetH
>
> Got it, thanks!
>
> Regards,
> Nick
>
> PS Forgot to reply to the list, sorry.

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


_______________________________________________
gdal-dev mailing list
[hidden email]
http://lists.osgeo.org/mailman/listinfo/gdal-dev