Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use a singleton pattern to register the gdal driver once in one place #4628

Merged
merged 6 commits into from Mar 13, 2024
Merged

use a singleton pattern to register the gdal driver once in one place #4628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
733a332
fixes #4623
kevinkreiser Mar 13, 2024
56c6a5e
Merge remote-tracking branch 'origin/master' into kk_initgdalsingleton
kevinkreiser Mar 13, 2024
c8ff6d9
lint
kevinkreiser Mar 13, 2024
6b81a83
changelog and unbreak gdal off
kevinkreiser Mar 13, 2024
da5c7de
Update CMakeLists.txt
kevinkreiser Mar 13, 2024
82e11dd
technically dont need these but we're relying on them happening somew…
kevinkreiser Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@
* FIXED: Use helper function for only parsing out names from DirectedEdge when populating intersecting edges [#4604](https://github.com/valhalla/valhalla/pull/4604)
* FIXED: Osmnode size reduction: Fixed excessive disk space for planet build [#4605](https://github.com/valhalla/valhalla/pull/4605)
* FIXED: CostMatrix for trivial routes with oneways [#4626](https://github.com/valhalla/valhalla/pull/4626)
* FIXED: some entry points to creating geotiff isochrones output did not register the geotiff driver before attempting to use it [#4628](https://github.com/valhalla/valhalla/pull/4628)
* **Enhancement**
* UPDATED: French translations, thanks to @xlqian [#4159](https://github.com/valhalla/valhalla/pull/4159)
* CHANGED: -j flag for multithreaded executables to override mjolnir.concurrency [#4168](https://github.com/valhalla/valhalla/pull/4168)
Expand Down
17 changes: 11 additions & 6 deletions CMakeLists.txt
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ option(ENABLE_SINGLE_FILES_WERROR "Convert compiler warnings to errors for singl
option(PREFER_EXTERNAL_DEPS "Whether to use internally vendored headers or find the equivalent external package" OFF)
# useful to workaround issues likes this https://stackoverflow.com/questions/24078873/cmake-generated-xcode-project-wont-compile
option(ENABLE_STATIC_LIBRARY_MODULES "If ON builds Valhalla modules as STATIC library targets" OFF)
option(ENABLE_GDAL "Whether to include GDAL; currently only used for raster serialization of isotile grid" OFF)
option(ENABLE_GDAL "Whether to include GDAL; currently only used for raster serialization of isotile grid" ON)

set(LOGGING_LEVEL "" CACHE STRING "Logging level, default is INFO")
set_property(CACHE LOGGING_LEVEL PROPERTY STRINGS "NONE;ALL;ERROR;WARN;INFO;DEBUG;TRACE")
Expand Down Expand Up @@ -191,12 +191,17 @@ else()
message(FATAL_ERROR "Required target protobuf::libprotobuf-lite or protobuf::libprotobuf is not defined")
endif()

# gdal
set(gdal_target "")
# unless you said you didnt want gdal we try to turn it on, if we cant we tell you
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought it would be better to default turning it ON if we can and if we couldnt we simply report that was the case. if you tell it you definitely dont want it by setting it to OFF then we dont even try

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm but doesn’t OFF still want to pull in GDAL::GDAL in tyr’s cmakelists? That’s what that variable was for, though you could handle that with a cmake expression in tyr as well, that’s probably more explicit than the previous var

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep will do that real quick

set(GDAL_TARGET "")
if (ENABLE_GDAL)
find_package(GDAL REQUIRED)
add_compile_definitions(ENABLE_GDAL)
set(gdal_target GDAL::GDAL)
find_package(GDAL QUIET)
if (GDAL_FOUND)
set(GDAL_TARGET GDAL::GDAL)
add_compile_definitions(ENABLE_GDAL)
message(STATUS "Gdal support is enabled")
else()
message(WARNING "Unable to enable gdal support")
endif()
endif()

set(CMAKE_FIND_PACKAGE_PREFER_CONFIG OFF)
Expand Down
6 changes: 2 additions & 4 deletions src/tyr/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -20,8 +20,6 @@ set(system_includes
${date_include_dir}
$<$<BOOL:${WIN32}>:${dirent_include_dir}>)



valhalla_module(NAME tyr
SOURCES
${sources}
Expand All @@ -46,6 +44,6 @@ valhalla_module(NAME tyr
valhalla::odin
valhalla::proto
${valhalla_protobuf_targets}
Boost::boost
${gdal_target}
Boost::boost
${GDAL_TARGET}
)
19 changes: 18 additions & 1 deletion src/tyr/isochrone_serializer.cc
View file
Open in desktop
Expand Up @@ -16,6 +16,23 @@
using namespace valhalla::baldr::json;

namespace {

// allows us to only ever register the driver once per process without having to put it
// in every executable that might call into this code
struct gdal_singleton_t {
static const gdal_singleton_t& get() {
static const gdal_singleton_t instance;
return instance;
}

private:
gdal_singleton_t() {
#ifdef ENABLE_GDAL
GDALRegister_GTiff();
#endif
}
};

using rgba_t = std::tuple<float, float, float>;

using namespace valhalla;
Expand Down Expand Up @@ -169,7 +186,7 @@ std::string serializeGeoTIFF(Api& request, const std::shared_ptr<const GriddedDa
char** geotiff_options = NULL;
geotiff_options = CSLSetNameValue(geotiff_options, "COMPRESS", "PACKBITS");

// TODO: check if there's an easy way to only instantiate the driver once
gdal_singleton_t::get();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this first time this is called (per process) the constructor above is called and the driver registration happens. subsequent calls are no-ops

auto driver_manager = GetGDALDriverManager();
auto geotiff_driver = driver_manager->GetDriverByName("GTiff");
auto geotiff_dataset =
Expand Down
9 changes: 0 additions & 9 deletions src/valhalla_run_isochrone.cc
View file
Open in desktop
Expand Up @@ -20,10 +20,6 @@
#include "tyr/serializers.h"
#include "worker.h"

#ifdef ENABLE_GDAL
#include <gdal_priv.h>
#endif

using namespace valhalla;
using namespace valhalla::midgard;
using namespace valhalla::baldr;
Expand Down Expand Up @@ -84,11 +80,6 @@ int main(int argc, char* argv[]) {
Api request;
ParseApi(json_str, valhalla::Options::isochrone, request);

#ifdef ENABLE_GDAL
if (request.options().format() == Options_Format_geotiff) {
GDALRegister_GTiff();
}
#endif
auto& options = *request.mutable_options();

// Get the denoise parameter
Expand Down
8 changes: 0 additions & 8 deletions src/valhalla_service.cc
View file
Open in desktop
Expand Up @@ -17,10 +17,6 @@
using namespace prime_server;
#endif

#ifdef ENABLE_GDAL
#include <gdal_priv.h>
#endif

#include "config.h"
#include "midgard/logging.h"

Expand All @@ -43,10 +39,6 @@ int main(int argc, char** argv) {
}
#endif

#ifdef ENABLE_GDAL
GDALRegister_GTiff();
#endif

// config file
// TODO: validate the config
std::string config_file(argv[1]);
Expand Down
3 changes: 0 additions & 3 deletions test/isochrone.cc
View file
Open in desktop
Expand Up @@ -355,7 +355,6 @@ void check_raster_edges(size_t x, size_t y, uint16_t* data) {
}

TEST(Isochrones, test_geotiff_output_distance) {
GDALRegister_GTiff();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont really like removing these because the tests themselves use gdal directly to load the response from the library. now in this case technically we can remove them because hidden in the source of the serializer we are doing this initialization but the tests (and just in general any calling code) shouldnt know/assume that its preconditions are met by something it calls into. the only way to get around this would be to expose gdal initialization as a public api in the library and have the tests call that but i hate that idea even worse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn’t check that.. I kinda assumed those driver inits were there to make the test work (it wouldn’t have without). That’s true.. anyways, the alternative to test this bit is indeed worse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other idea is to leave them there though what i would do is in the suite initialization call it once

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both is fine with me tbh, I get your point, but also think it’ll be fine probably forever ®

loki_worker_t loki_worker(cfg);
thor_worker_t thor_worker(cfg);

Expand Down Expand Up @@ -401,7 +400,6 @@ TEST(Isochrones, test_geotiff_output_distance) {
}

TEST(Isochrones, test_geotiff_output_time) {
GDALRegister_GTiff();
loki_worker_t loki_worker(cfg);
thor_worker_t thor_worker(cfg);

Expand Down Expand Up @@ -448,7 +446,6 @@ TEST(Isochrones, test_geotiff_output_time) {

// test request with two metrics
TEST(Isochrones, test_geotiff_output_time_distance) {
GDALRegister_GTiff();
loki_worker_t loki_worker(cfg);
thor_worker_t thor_worker(cfg);

Expand Down