Skip to content

Conversation

@nono303
Copy link

@nono303 nono303 commented Aug 2, 2023

Implement some GDAL functions:

  • gdalregisterall
  • gdalopen
  • gdal_locationinfo
  • gdal_ds_getsrscode
  • gdal_tr_create

summary of changes

config.w32

  • Allow use of shared lib
  • Add gdal.h

php_ogr_api.h

  • Add requested definition for new functions

php_ogr.h

  • PHP_OGR_VERSION 1.6.1 > 1.7.0
  • Define PHP_OGR_EXTNAME (used in ogr_module_entry)

ogr.c

  • Implement
    • New GDAL functions
    • gdal_free_Dataset() destructor
  • Fix possible null pointer in ogr_free_Datasource
  • Reorder Error handling functions (CPLSetErrorHandler) before REGISTER_INI_ENTRIES
  • PHP_MSHUTDOWN_FUNCTION : call GDALDumpOpenDatasets & GDALDestroyDriverManager (:warning: need to be verified…)
  • PHP_MINFO_FUNCTION : add GDAL Build Info
  • Add get_ogr_error_string
  • Fix
    • Proto name & typo
    • Tab2space

To be done

  • Tests & examples for new functions
  • Add gdal.h in config.m4

And don't hesitate to give me some feedback / criticism as I'm quite rusty in C, I would enjoy to improve it!

nono303 added 18 commits January 31, 2023 16:33
add header gdal.h
check php version
add message if failure
add get_ogr_error_string()
fix null pointer exceptions
fix optional epsg default parameter issue
fix OGRSpatialReference definition
…s...)

remove gdal_free (useless as gdal_free_Dataset now do the job)
fix rasterSRS value
- properties into specific array
- add epsg code
raster properties
- add all (not just first) 'raster' file(s) with value into specific array
- comment projectionRef
…ocationinfo:

- add gdal_tr_create (return CoordinateTransformation resource) & gdal_ds_getsrscode (get dataset SRS code)
- gdalopen: add optional passed by reference trgSRS parameter to retrieve dataset SRS code at Dataset creation
- gdal_locationinfo: change int epsg additional parameter from  to zhct CoordinateTransformation
fix:
- OSRGetAuthorityCode: COMPD_CS > NULL (first in root)
- avoid useless resource creation: OSRNewSpatialReference(GDALGetProjectionRef(GDALDatasetH)) > GDALGetSpatialRef(GDALDatasetH)
- add RETURN_NULL() after zend_throw_exception
add correct proto to gdal* functions
@ejn
Copy link
Collaborator

ejn commented Aug 15, 2023

First of all: Many thanks for this - looks like some really good work and a first step to getting at least a little bit of the GDAL raster handling in to PHP so I'd be very interested in accepting this PR.

I haven't actually tested anything yet, so the following comments are based purely on a brief code review and are intended as discussion points - I appreciate that you probably did this to solve a specific problem and may or may not have some capacity to generalise things.

  1. The intention of a simplified gdal_open with an SRID parameter is understandable, but I think this is probably too simplified - GDAL supports arbitrary spatial reference systems for good reasons, and it also seems a shame not to expose the additional arguments to GDALOpenEX. If GDALGetSpatialRef and GDALSetSpatialRef would be exposed, it would be possible for the user to set the CRS manually if necessary.
  2. Instead of gdal_ds_getsrscode it would be better to expose GDALGetSpatialRef directly - the code can then be obtained using the existing osr_getauthoritycode (or indeed all other information relating to the SRS using the extensive osr_-functions).
  3. gdal_tr_create is a nice shorthand wrapper, but so far as I can see then apart from the calls to OCTNewCoordinateTransformation and OSRSetAxisMappingStrategy this could all be implemented in PHP using the API already exposed as osr_-functions. It is also (i) restricted to only CRS which can be identified by EPSG code and (ii) always sets OAMS_TRADITIONAL_GIS_ORDER, which reduce the general-purpose usefulness. Would you instead consider exposing the two missing functions and re-writing this function (maybe as an example) in PHP?
  4. gdal_locationinfo is also a nice shorthand, but particularly the limitation to the first band seems arbitrary. Maybe it would be possibly to add a second (optional: Default 1) parameter to the function to specify the band (and also maybe directly expose GDALGetRasterCount so that the sensible values can be determined in advance)? As a minor note: the returned epsg entry contains the authority code, but it is not checked that the authority is EPSG.

For any functions such as gdal_locationinfo which aren't (at least more or less) taken 1:1 from the GDAL C API it would be good to have some documentation (at least input/output parameter) - tests and examples you already listed as TODOs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants