From 18287be1dbd25e98d3ac59e13a86687c5e697605 Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Tue, 10 May 2016 15:24:46 +0300 Subject: [PATCH] [coding] Fixes to SRTM parser CL. --- coding/zip_reader.cpp | 36 +++--- coding/zip_reader.hpp | 6 +- .../generator_tests/srtm_parser_test.cpp | 24 ++-- generator/srtm_parser.cpp | 117 ++++++++++++++---- generator/srtm_parser.hpp | 76 ++++++------ .../srtm_source_tests/srtm_source_tests.cpp | 18 +-- .../srtm_source_tests/srtm_source_tests.pro | 4 +- 7 files changed, 177 insertions(+), 104 deletions(-) diff --git a/coding/zip_reader.cpp b/coding/zip_reader.cpp index 4c37d66090..102bdd77a4 100644 --- a/coding/zip_reader.cpp +++ b/coding/zip_reader.cpp @@ -118,30 +118,28 @@ void ZipFileReader::UnzipFile(string const & zipContainer, string const & fileIn outFileGuard.release(); } -char* ZipFileReader::UnzipFileToMemory(string const & zipContainer, string const & fileInZip) +void ZipFileReader::UnzipFileToMemory(string const & cont, string const & file, + vector & data) { - unzFile zip = unzOpen64(zipContainer.c_str()); + unzFile zip = unzOpen64(cont.c_str()); if (!zip) - MYTHROW(OpenZipException, ("Can't get zip file handle", zipContainer)); - MY_SCOPE_GUARD(zipGuard, bind(&unzClose, zip)); - - if (UNZ_OK != unzLocateFile(zip, fileInZip.c_str(), 1)) - MYTHROW(LocateZipException, ("Can't locate file inside zip", fileInZip)); + MYTHROW(OpenZipException, ("Can't get zip file handle:", cont)); + MY_SCOPE_GUARD(zipCloser, bind(&unzClose, zip)); + if (UNZ_OK != unzLocateFile(zip, file.c_str(), 1 /* case sensitivity */)) + MYTHROW(LocateZipException, ("Can't locate file inside zip container:", file)); if (UNZ_OK != unzOpenCurrentFile(zip)) - MYTHROW(LocateZipException, ("Can't open file inside zip", fileInZip)); - MY_SCOPE_GUARD(currentFileGuard, bind(&unzCloseCurrentFile, zip)); + MYTHROW(LocateZipException, ("Can't open file inside zip container:", file)); + MY_SCOPE_GUARD(currentFileCloser, bind(&unzCloseCurrentFile, zip)); - unz_file_info64 fileInfo; - if (UNZ_OK != unzGetCurrentFileInfo64(zip, &fileInfo, NULL, 0, NULL, 0, NULL, 0)) - MYTHROW(LocateZipException, ("Can't get uncompressed file size inside zip", fileInZip)); - size_t const uncompressedFileSize = fileInfo.uncompressed_size; + unz_file_info64 info; + if (UNZ_OK != unzGetCurrentFileInfo64(zip, &info, NULL, 0, NULL, 0, NULL, 0)) + MYTHROW(LocateZipException, ("Can't get uncompressed file size inside zip:", file)); - char* buf= new char[uncompressedFileSize]; + size_t const size = info.uncompressed_size; + data.resize(size); - int const readBytes = unzReadCurrentFile(zip, buf, uncompressedFileSize); - if (readBytes < 0) - MYTHROW(InvalidZipException, ("Error", readBytes, "while unzipping", fileInZip, "from", zipContainer)); - - return buf; + int const bytesRead = unzReadCurrentFile(zip, data.data(), size); + if (bytesRead < 0) + MYTHROW(InvalidZipException, ("Error:", bytesRead, "while unzipping", file, "in", cont)); } diff --git a/coding/zip_reader.hpp b/coding/zip_reader.hpp index 6c36693df4..7970cff087 100644 --- a/coding/zip_reader.hpp +++ b/coding/zip_reader.hpp @@ -32,8 +32,10 @@ public: static void UnzipFile(string const & zipContainer, string const & fileInZip, string const & outFilePath, ProgressFn progressFn = ProgressFn()); - /// @warning Counsumer must manually free result - static char* UnzipFileToMemory(string const & zipContainer, string const & fileInZip); + /// Unzips |file| in |cont| to |buffer|. + /// + /// @warning Can throw OpenZipException and LocateZipException. + static void UnzipFileToMemory(string const & cont, string const & file, vector & data); static void FilesList(string const & zipContainer, FileListT & filesList); diff --git a/generator/generator_tests/srtm_parser_test.cpp b/generator/generator_tests/srtm_parser_test.cpp index 7c72db3681..4420ef91a7 100644 --- a/generator/generator_tests/srtm_parser_test.cpp +++ b/generator/generator_tests/srtm_parser_test.cpp @@ -1,22 +1,28 @@ -#include "generator/srtm_parser.hpp" - #include "testing/testing.hpp" -#include "base/logging.hpp" +#include "generator/srtm_parser.hpp" + +using namespace generator; namespace { +string GetBase(ms::LatLon const & coord) { return SrtmFile::GetBase(coord); } + UNIT_TEST(FilenameTests) { - auto name = GetSRTMBase({56.4566, 37.3467}); + auto name = GetBase({56.4566, 37.3467}); TEST_EQUAL(name, "N56E037", ()); - name = GetSRTMBase({34.077433, -118.304569}); + + name = GetBase({34.077433, -118.304569}); TEST_EQUAL(name, "N34W119", ()); - name = GetSRTMBase({0.1, 0.1}); + + name = GetBase({0.1, 0.1}); TEST_EQUAL(name, "N00E000", ()); - name = GetSRTMBase({-35.35, -12.1}); + + name = GetBase({-35.35, -12.1}); TEST_EQUAL(name, "S36W013", ()); - name = GetSRTMBase({-34.622358, -58.383654}); + + name = GetBase({-34.622358, -58.383654}); TEST_EQUAL(name, "S35W059", ()); } -} +} // namespace diff --git a/generator/srtm_parser.cpp b/generator/srtm_parser.cpp index ff88a35512..6630c335de 100644 --- a/generator/srtm_parser.cpp +++ b/generator/srtm_parser.cpp @@ -1,12 +1,74 @@ -#include "srtm_parser.hpp" +#include "generator/srtm_parser.hpp" +#include "coding/endianness.hpp" #include "coding/zip_reader.hpp" -#include "std/iomanip.hpp" +#include "base/logging.hpp" -string GetSRTMBase(ms::LatLon coord) +#include "std/iomanip.hpp" +#include "std/sstream.hpp" + +namespace generator { - stringstream ss; +// SrtmFile ---------------------------------------------------------------------------------------- +SrtmFile::SrtmFile() +{ + Invalidate(); +} + +SrtmFile::SrtmFile(SrtmFile && rhs) : m_data(move(rhs.m_data)), m_valid(rhs.m_valid) +{ + rhs.Invalidate(); +} + +void SrtmFile::Init(string const & dir, ms::LatLon const & coord) +{ + Invalidate(); + + string const base = GetBase(coord); + string const cont = dir + base + ".SRTMGL1.hgt.zip"; + string file = base + ".hgt"; + + try + { + ZipFileReader::UnzipFileToMemory(cont, file, m_data); + } + catch (ZipFileReader::LocateZipException e) + { + // Sometimes packed file has different name. See N39E051 measure. + file = base + ".SRTMGL1.hgt"; + ZipFileReader::UnzipFileToMemory(cont, file, m_data); + } + + m_valid = true; +} + +SrtmFile::THeight SrtmFile::GetHeight(ms::LatLon const & coord) +{ + if (!IsValid()) + return kInvalidHeight; + + double ln = coord.lon - static_cast(coord.lon); + if (ln < 0) + ln += 1; + double lt = coord.lat - static_cast(coord.lat); + if (lt < 0) + lt += 1; + lt = 1 - lt; // from North to South + + size_t const row = 3600 * lt; + size_t const col = 3600 * ln; + + size_t const ix = row * 3601 + col; + + if (ix >= Size()) + return kInvalidHeight; + return ReverseByteOrder(Data()[ix]); +} + +string SrtmFile::GetBase(ms::LatLon coord) +{ + ostringstream ss; if (coord.lat < 0) { ss << "S"; @@ -17,7 +79,7 @@ string GetSRTMBase(ms::LatLon coord) { ss << "N"; } - ss << setw(2) << setfill('0') << int(coord.lat); + ss << setw(2) << setfill('0') << static_cast(coord.lat); if (coord.lon < 0) { @@ -29,34 +91,41 @@ string GetSRTMBase(ms::LatLon coord) { ss << "E"; } - ss << setw(3) << int(coord.lon); + ss << setw(3) << static_cast(coord.lon); return ss.str(); } -void SrtmFile::Init(string dir, ms::LatLon coord) +void SrtmFile::Invalidate() { - string base = GetSRTMBase(coord); - string path = dir + base + ".SRTMGL1.hgt.zip"; - string file = base + ".hgt"; - try - { - m_data = reinterpret_cast(ZipFileReader::UnzipFileToMemory(path, file)); - } - catch (ZipFileReader::OpenZipException e) - { - m_data = nullptr; - } - catch (ZipFileReader::LocateZipException e) + m_data.clear(); + m_data.shrink_to_fit(); + m_valid = false; +} + +// SrtmFileManager --------------------------------------------------------------------------------- +SrtmFileManager::SrtmFileManager(string const & dir) : m_dir(dir) {} + +SrtmFile::THeight SrtmFileManager::GetHeight(ms::LatLon const & coord) +{ + string const base = SrtmFile::GetBase(coord); + auto it = m_storage.find(base); + if (it == m_storage.end()) { + SrtmFile file; try { - // Sometimes packed file has different name. See N39E051 measure. - file = base + ".SRTMGL1.hgt"; - m_data = reinterpret_cast(ZipFileReader::UnzipFileToMemory(path, file)); + file.Init(m_dir, coord); } - catch (int) + catch (RootException const & e) { - m_data = nullptr; + LOG(LERROR, ("Can't init SRTM file:", base, "reason:", e.Msg())); } + + // It's OK to store even invalid files and return invalid height + // for them later. + m_storage.emplace(base, move(file)); } + + return m_storage[base].GetHeight(coord); } +} // namespace generator diff --git a/generator/srtm_parser.hpp b/generator/srtm_parser.hpp index d01b33f153..bf521ad988 100644 --- a/generator/srtm_parser.hpp +++ b/generator/srtm_parser.hpp @@ -2,64 +2,58 @@ #include "geometry/latlon.hpp" -#include "coding/endianness.hpp" +#include "base/macros.hpp" -#include "std/map.hpp" +#include "std/cstdint.hpp" #include "std/string.hpp" +#include "std/unordered_map.hpp" +#include "std/vector.hpp" -using TSRTMHeighType = int16_t; - -int16_t constexpr kInvalidSRTMHeight = -32768; - -string GetSRTMBase(ms::LatLon coord); - +namespace generator +{ class SrtmFile { public: - void Init(string dir, ms::LatLon coord); + using THeight = int16_t; - TSRTMHeighType getHeight(ms::LatLon coord) - { - if (m_data == nullptr) - return kInvalidSRTMHeight; - double ln = coord.lon - int(coord.lon); - if (ln < 0) - ln += 1; - double lt = coord.lat - int(coord.lat); - if (lt < 0) - lt += 1; - lt = 1 - lt; // From North to South + static THeight constexpr kInvalidHeight = -32768; - size_t const row = 3600 * lt; - size_t const column = 3600 * ln; - return ReverseByteOrder(m_data[row * 3601 + column]); - // 3600 and 3601 because 3601 is owerlap column. - } + SrtmFile(); + SrtmFile(SrtmFile && rhs); - ~SrtmFile() - { - if (m_data) - delete[] m_data; - } + void Init(string const & dir, ms::LatLon const & coord); + + inline bool IsValid() const { return m_valid; } + + // Returns height in meters at |coord|, or kInvalidHeight if is not initialized. + THeight GetHeight(ms::LatLon const & coord); + + static string GetBase(ms::LatLon coord); private: - TSRTMHeighType * m_data; + inline THeight const * Data() const { return reinterpret_cast(m_data.data()); }; + + inline size_t Size() const { return m_data.size() / sizeof(THeight); } + + void Invalidate(); + + vector m_data; + bool m_valid; + + DISALLOW_COPY(SrtmFile); }; class SrtmFileManager { public: - SrtmFileManager(string dir) : m_dir(dir) {} - TSRTMHeighType GetCoordHeight(ms::LatLon coord) - { - string base = GetSRTMBase(coord); - auto it = m_storage.find(base); - if (it == m_storage.end()) - m_storage[base].Init(m_dir, coord); - return m_storage[base].getHeight(coord); - } + SrtmFileManager(string const & dir); + + SrtmFile::THeight GetHeight(ms::LatLon const & coord); private: string m_dir; - map m_storage; + unordered_map m_storage; + + DISALLOW_COPY(SrtmFileManager); }; +} // namespace generator diff --git a/generator/srtm_source_tests/srtm_source_tests.cpp b/generator/srtm_source_tests/srtm_source_tests.cpp index 8c09903c2c..5895b24bc5 100644 --- a/generator/srtm_source_tests/srtm_source_tests.cpp +++ b/generator/srtm_source_tests/srtm_source_tests.cpp @@ -6,16 +6,16 @@ #include "routing/routing_integration_tests/routing_test_tools.hpp" +#include "coding/file_name_utils.hpp" + #include "platform/country_file.hpp" #include "platform/local_country_file_utils.hpp" -#include "coding/file_name_utils.hpp" - namespace { #define SRTM_SOURCES_PATH "srtm/e4ftl01.cr.usgs.gov/SRTM/SRTMGL1.003/2000.02.11/" -UNIT_TEST(SRTMCoverageChecker) +UNIT_TEST(SrtmCoverageChecker) { vector localFiles; platform::FindAllLocalMapsAndCleanup(numeric_limits::max() /* latestVersion */, @@ -47,7 +47,9 @@ UNIT_TEST(SRTMCoverageChecker) segMapping.Map(container); Platform & platform = GetPlatform(); - SrtmFileManager manager(my::JoinFoldersToPath(platform.WritableDir(), SRTM_SOURCES_PATH)); + generator::SrtmFileManager manager( + my::JoinFoldersToPath(platform.WritableDir(), SRTM_SOURCES_PATH)); + size_t all = 0; size_t good = 0; @@ -83,11 +85,11 @@ UNIT_TEST(SRTMCoverageChecker) for (int64_t idx = startIdx; idx >= static_cast(endIdx); --idx) path.push_back(ft.GetPoint(idx)); } - for (auto point : path) + for (auto const & point : path) { - auto height = manager.GetCoordHeight(MercatorBounds::ToLatLon(point)); + auto const height = manager.GetHeight(MercatorBounds::ToLatLon(point)); all++; - if (height != kInvalidSRTMHeight) + if (height != generator::SrtmFile::kInvalidHeight) good++; } } @@ -95,8 +97,10 @@ UNIT_TEST(SRTMCoverageChecker) auto const delta = all - good; auto const percent = delta * 100.0 / all; if (percent > 10.0) + { LOG(LINFO, ("Huge error rate in:", file.GetCountryName(), "Good:", good, "All:", all, "Delta:", delta, "%:", percent)); + } } } } // namespace diff --git a/generator/srtm_source_tests/srtm_source_tests.pro b/generator/srtm_source_tests/srtm_source_tests.pro index 4d0ff89d59..00d9b7d7c0 100644 --- a/generator/srtm_source_tests/srtm_source_tests.pro +++ b/generator/srtm_source_tests/srtm_source_tests.pro @@ -7,8 +7,8 @@ CONFIG -= app_bundle TEMPLATE = app ROOT_DIR = ../.. -DEPENDENCIES = map routing search storage indexer platform editor geometry coding base \ - osrm jansson protobuf tomcrypt succinct stats_client pugixml generator minizip +DEPENDENCIES = generator map routing search storage indexer platform editor geometry coding base \ + osrm jansson protobuf tomcrypt succinct stats_client pugixml minizip macx-*: LIBS *= "-framework IOKit" "-framework SystemConfiguration"