From efaea41c5615b542a61c9ecf3a604b7d8cdedfd1 Mon Sep 17 00:00:00 2001 From: Tim Sylvester Date: Tue, 30 Apr 2024 09:12:41 -0700 Subject: [PATCH] Dense image maps (#2098) --- .../icon-sdf-non-sdf-one-layer/metrics.json | 6 ++--- platform/android/DEVELOPING.md | 4 +-- render-test/manifest_parser.cpp | 26 ++++++++++++++++++- render-test/runner.cpp | 14 +++++++--- src/mbgl/layout/layout.hpp | 4 +-- src/mbgl/renderer/image_atlas.cpp | 23 +++++++++------- src/mbgl/renderer/image_atlas.hpp | 6 ++--- src/mbgl/renderer/image_manager.cpp | 4 +++ src/mbgl/renderer/image_manager.hpp | 1 + src/mbgl/style/image_impl.hpp | 9 +++---- test/renderer/image_manager.test.cpp | 12 ++++----- 11 files changed, 74 insertions(+), 35 deletions(-) diff --git a/metrics/android-render-test-runner/render-tests/icon-image/icon-sdf-non-sdf-one-layer/metrics.json b/metrics/android-render-test-runner/render-tests/icon-image/icon-sdf-non-sdf-one-layer/metrics.json index 9689f9840..dd3415d17 100644 --- a/metrics/android-render-test-runner/render-tests/icon-image/icon-sdf-non-sdf-one-layer/metrics.json +++ b/metrics/android-render-test-runner/render-tests/icon-image/icon-sdf-non-sdf-one-layer/metrics.json @@ -19,8 +19,8 @@ 13, 1, [ - 26720, - 26720 + 26600, + 26600 ], [ 46, @@ -32,4 +32,4 @@ ] ] ] -} \ No newline at end of file +} diff --git a/platform/android/DEVELOPING.md b/platform/android/DEVELOPING.md index d45a2e9d6..90b630ae9 100644 --- a/platform/android/DEVELOPING.md +++ b/platform/android/DEVELOPING.md @@ -55,7 +55,7 @@ The instrumentation tests are running on AWS Device Farm. To see the results and https://us-west-2.console.aws.amazon.com/devicefarm/home?region=us-east-1#/mobile/projects/20687d72-0e46-403e-8f03-0941850665bc/runs -You can log with the `maplibre` alias, with `maplibre` as username and `maplibre` as password (this is a read-only account). +You can log in with the `maplibre` alias, with `maplibre` as username and `maplibre` as password (this is a read-only account). ## Kotlin @@ -75,4 +75,4 @@ To format all Kotlin source files, use: ``` $ ./gradlew formatKotlin -``` \ No newline at end of file +``` diff --git a/render-test/manifest_parser.cpp b/render-test/manifest_parser.cpp index 0fb740caa..c0c205704 100644 --- a/render-test/manifest_parser.cpp +++ b/render-test/manifest_parser.cpp @@ -287,7 +287,31 @@ std::optional ManifestParser::parseManifest(const std::string& manifes testId = testId.substr(rootLength + 1, testId.length() - rootLength - 1); std::vector expectedMetricPaths{expectedMetricPath}; -#if defined(__APPLE__) +#if defined(__ANDROID__) + // todo: use `Context.getExternalFilesDir()` or similar via JNI to select an appropriate destination + const auto locations = std::vector{ + "/sdcard", + "/storage/emulated/0", + "/storage/self/primary", + }; + static bool reportedOnce = false; + for (const auto& location : locations) { + // Checking `mbgl::filesystem::status` doesn't accurately reflect whether we can create subdirectories, + // so just try it. (See `TestRunner::checkProbingResults`) + try { + const auto baselinesPath = location + "/baselines"; + mbgl::filesystem::create_directories(baselinesPath); + expectedMetricPaths.emplace_back(baselinesPath); + break; + } catch (mbgl::filesystem::filesystem_error& ex) { + if (!reportedOnce) { + mbgl::Log::Warning(mbgl::Event::Android, "Not a writable directory: " + std::string(ex.what())); + } + } + } + // Only log on the first case + reportedOnce = true; +#elif defined(__APPLE__) expectedMetricPaths.emplace_back(manifest.manifestPath + "/baselines/"); #endif testPaths.emplace_back(testPath, diff --git a/render-test/runner.cpp b/render-test/runner.cpp index 38e34335e..330300c56 100644 --- a/render-test/runner.cpp +++ b/render-test/runner.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -285,9 +286,16 @@ void TestRunner::checkProbingResults(TestMetadata& resultMetadata) { if (resultMetadata.metrics.isEmpty()) return; const auto writeMetrics = [&resultMetadata](const mbgl::filesystem::path& path, const std::string& message = std::string()) { - mbgl::filesystem::create_directories(path); - mbgl::util::write_file(path / "metrics.json", serializeMetrics(resultMetadata.metrics)); - resultMetadata.errorMessage += message; + try { + mbgl::filesystem::create_directories(path); + mbgl::util::write_file(path / "metrics.json", serializeMetrics(resultMetadata.metrics)); + resultMetadata.errorMessage += message; + } catch (mbgl::filesystem::filesystem_error& ex) { + const auto msg = "Failed to write metrics. path='" + path.string() + "' err='" + ex.what() + + "' msg=" + message; + resultMetadata.errorMessage += msg; + Log::Warning(Event::General, msg); + } }; const std::vector& expectedMetrics = resultMetadata.paths.expectedMetrics; diff --git a/src/mbgl/layout/layout.hpp b/src/mbgl/layout/layout.hpp index e225aa4bd..b826881b9 100644 --- a/src/mbgl/layout/layout.hpp +++ b/src/mbgl/layout/layout.hpp @@ -25,9 +25,9 @@ public: bool, const CanonicalTileID&) = 0; - virtual void prepareSymbols(const GlyphMap&, const GlyphPositions&, const ImageMap&, const ImagePositions&) {}; + virtual void prepareSymbols(const GlyphMap&, const GlyphPositions&, const ImageMap&, const ImagePositions&) {} - virtual bool hasSymbolInstances() const { return true; }; + virtual bool hasSymbolInstances() const { return true; } virtual bool hasDependencies() const = 0; }; diff --git a/src/mbgl/renderer/image_atlas.cpp b/src/mbgl/renderer/image_atlas.cpp index dcd78f58f..005376bbc 100644 --- a/src/mbgl/renderer/image_atlas.cpp +++ b/src/mbgl/renderer/image_atlas.cpp @@ -46,15 +46,18 @@ namespace { void populateImagePatches(ImagePositions& imagePositions, const ImageManager& imageManager, std::vector& /*out*/ patches) { + if (imagePositions.empty()) { + imagePositions.reserve(imageManager.updatedImageVersions.size()); + } for (auto& updatedImageVersion : imageManager.updatedImageVersions) { const std::string& name = updatedImageVersion.first; const uint32_t version = updatedImageVersion.second; - auto it = imagePositions.find(updatedImageVersion.first); + const auto it = imagePositions.find(updatedImageVersion.first); if (it != imagePositions.end()) { auto& position = it->second; if (position.version == version) continue; - auto updatedImage = imageManager.getSharedImage(name); + const auto updatedImage = imageManager.getSharedImage(name); if (updatedImage == nullptr) continue; patches.emplace_back(*updatedImage, position.paddedRect); @@ -72,28 +75,30 @@ std::vector ImageAtlas::getImagePatchesAndUpdateVersions(const Image return imagePatches; } -ImageAtlas makeImageAtlas(const ImageMap& icons, - const ImageMap& patterns, - const std::unordered_map& versionMap) { +ImageAtlas makeImageAtlas(const ImageMap& icons, const ImageMap& patterns, const ImageVersionMap& versionMap) { ImageAtlas result; mapbox::ShelfPack::ShelfPackOptions options; options.autoResize = true; mapbox::ShelfPack pack(0, 0, options); + result.iconPositions.reserve(icons.size()); + for (const auto& entry : icons) { const style::Image::Impl& image = *entry.second; const mapbox::Bin& bin = _packImage(pack, image, result, ImageType::Icon); - auto it = versionMap.find(entry.first); - auto version = it != versionMap.end() ? it->second : 0; + const auto it = versionMap.find(entry.first); + const auto version = it != versionMap.end() ? it->second : 0; result.iconPositions.emplace(image.id, ImagePosition{bin, image, version}); } + result.patternPositions.reserve(patterns.size()); + for (const auto& entry : patterns) { const style::Image::Impl& image = *entry.second; const mapbox::Bin& bin = _packImage(pack, image, result, ImageType::Pattern); - auto it = versionMap.find(entry.first); - auto version = it != versionMap.end() ? it->second : 0; + const auto it = versionMap.find(entry.first); + const auto version = it != versionMap.end() ? it->second : 0; result.patternPositions.emplace(image.id, ImagePosition{bin, image, version}); } diff --git a/src/mbgl/renderer/image_atlas.hpp b/src/mbgl/renderer/image_atlas.hpp index f27557dd5..aaaea1ba1 100644 --- a/src/mbgl/renderer/image_atlas.hpp +++ b/src/mbgl/renderer/image_atlas.hpp @@ -52,7 +52,7 @@ public: } }; -using ImagePositions = std::map; +using ImagePositions = mbgl::unordered_map; class ImagePatch { public: @@ -73,8 +73,6 @@ public: std::vector getImagePatchesAndUpdateVersions(const ImageManager&); }; -ImageAtlas makeImageAtlas(const ImageMap&, - const ImageMap&, - const std::unordered_map& versionMap); +ImageAtlas makeImageAtlas(const ImageMap&, const ImageMap&, const ImageVersionMap& versionMap); } // namespace mbgl diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index af08124b2..773fd2a64 100644 --- a/src/mbgl/renderer/image_manager.cpp +++ b/src/mbgl/renderer/image_manager.cpp @@ -285,6 +285,10 @@ void ImageManager::notify(ImageRequestor& requestor, const ImageRequestPair& pai ImageMap patternMap; ImageVersionMap versionMap; + iconMap.reserve(pair.first.size()); + patternMap.reserve(pair.first.size()); + versionMap.reserve(pair.first.size()); + for (const auto& dependency : pair.first) { auto it = images.find(dependency.first); if (it != images.end()) { diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp index 8890332da..8906d9725 100644 --- a/src/mbgl/renderer/image_manager.hpp +++ b/src/mbgl/renderer/image_manager.hpp @@ -5,6 +5,7 @@ #include #include +#include #include namespace mbgl { diff --git a/src/mbgl/style/image_impl.hpp b/src/mbgl/style/image_impl.hpp index bd47397ca..7983c5aa6 100644 --- a/src/mbgl/style/image_impl.hpp +++ b/src/mbgl/style/image_impl.hpp @@ -1,10 +1,9 @@ #pragma once #include +#include #include -#include -#include #include namespace mbgl { @@ -45,10 +44,10 @@ enum class ImageType : bool { Pattern }; -using ImageMap = std::unordered_map>; -using ImageDependencies = std::unordered_map; +using ImageMap = mbgl::unordered_map>; +using ImageDependencies = mbgl::unordered_map; using ImageRequestPair = std::pair; -using ImageVersionMap = std::unordered_map; +using ImageVersionMap = mbgl::unordered_map; inline bool operator<(const Immutable& a, const Immutable& b) { return a->id < b->id; } diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index c5d82527c..282b49822 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -86,12 +86,12 @@ public: void onImagesAvailable(ImageMap icons, ImageMap patterns, - std::unordered_map versionMap, + ImageVersionMap versionMap, uint64_t imageCorrelationID_) final { if (imagesAvailable && imageCorrelationID == imageCorrelationID_) imagesAvailable(icons, patterns, versionMap); } - std::function)> imagesAvailable; + std::function imagesAvailable; uint64_t imageCorrelationID = 0; }; @@ -105,7 +105,7 @@ TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { ImageManagerObserver observer; imageManager.setObserver(&observer); - requestor.imagesAvailable = [&](ImageMap, ImageMap, std::unordered_map) { + requestor.imagesAvailable = [&](ImageMap, ImageMap, ImageVersionMap) { notified = true; }; @@ -131,7 +131,7 @@ TEST(ImageManager, NotifiesRequestorImmediatelyIfDependenciesAreSatisfied) { StubImageRequestor requestor(imageManagerPtr); bool notified = false; - requestor.imagesAvailable = [&](ImageMap, ImageMap, std::unordered_map) { + requestor.imagesAvailable = [&](ImageMap, ImageMap, ImageVersionMap) { notified = true; }; @@ -173,7 +173,7 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { bool notified = false; - requestor.imagesAvailable = [&](ImageMap, ImageMap, std::unordered_map) { + requestor.imagesAvailable = [&](ImageMap, ImageMap, ImageVersionMap) { notified = true; }; @@ -229,7 +229,7 @@ TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { bool notified = false; - requestor.imagesAvailable = [&](ImageMap, ImageMap, std::unordered_map) { + requestor.imagesAvailable = [&](ImageMap, ImageMap, ImageVersionMap) { notified = true; };