From 9ebf727f56819f259a7a0771b1ed58b14c3adea3 Mon Sep 17 00:00:00 2001 From: Ivo van Dongen Date: Thu, 13 Jul 2017 15:35:02 +0300 Subject: [core] BackendScope prevent double (de-)activation - Guards against duplicate activations by checking wether the backend of the prior scope is the same as the current ones - Makes sure that only the most outer backend scope deactivates by tracking activation state --- cmake/test-files.cmake | 1 + include/mbgl/renderer/backend_scope.hpp | 4 ++ include/mbgl/renderer/renderer_backend.hpp | 5 ++ src/mbgl/renderer/backend_scope.cpp | 35 ++++++--- test/renderer/backend_scope.test.cpp | 111 +++++++++++++++++++++++++++++ 5 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 test/renderer/backend_scope.test.cpp diff --git a/cmake/test-files.cmake b/cmake/test-files.cmake index ca7182581b..2d2ed7d35b 100644 --- a/cmake/test-files.cmake +++ b/cmake/test-files.cmake @@ -41,6 +41,7 @@ set(MBGL_TEST_FILES test/programs/symbol_program.test.cpp # renderer + test/renderer/backend_scope.test.cpp test/renderer/group_by_layout.test.cpp test/renderer/image_manager.test.cpp diff --git a/include/mbgl/renderer/backend_scope.hpp b/include/mbgl/renderer/backend_scope.hpp index f8a258f3df..73bafc84c7 100644 --- a/include/mbgl/renderer/backend_scope.hpp +++ b/include/mbgl/renderer/backend_scope.hpp @@ -22,10 +22,14 @@ public: static bool exists(); private: + void activate(); + void deactivate(); + BackendScope* priorScope; BackendScope* nextScope; RendererBackend& backend; const ScopeType scopeType; + bool activated = false; }; } // namespace mbgl diff --git a/include/mbgl/renderer/renderer_backend.hpp b/include/mbgl/renderer/renderer_backend.hpp index 9d967fd51b..a383329395 100644 --- a/include/mbgl/renderer/renderer_backend.hpp +++ b/include/mbgl/renderer/renderer_backend.hpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -81,4 +82,8 @@ private: friend class BackendScope; }; +MBGL_CONSTEXPR bool operator==(const RendererBackend& a, const RendererBackend& b) { + return &a == &b; +} + } // namespace mbgl diff --git a/src/mbgl/renderer/backend_scope.cpp b/src/mbgl/renderer/backend_scope.cpp index 5d57421c48..fafeaabb39 100644 --- a/src/mbgl/renderer/backend_scope.cpp +++ b/src/mbgl/renderer/backend_scope.cpp @@ -16,30 +16,49 @@ BackendScope::BackendScope(RendererBackend& backend_, ScopeType scopeType_) if (priorScope) { assert(priorScope->nextScope == nullptr); priorScope->nextScope = this; + priorScope->deactivate(); } - if (scopeType == ScopeType::Explicit) { - backend.activate(); - } + + activate(); currentScope.set(this); } BackendScope::~BackendScope() { assert(nextScope == nullptr); + deactivate(); + if (priorScope) { - priorScope->backend.activate(); + priorScope->activate(); currentScope.set(priorScope); assert(priorScope->nextScope == this); priorScope->nextScope = nullptr; } else { - if (scopeType == ScopeType::Explicit) { - backend.deactivate(); - } - currentScope.set(nullptr); } } +void BackendScope::activate() { + if (scopeType == ScopeType::Explicit && + !(priorScope && this->backend == priorScope->backend) && + !(nextScope && this->backend == nextScope->backend)) { + // Only activate when set to Explicit and + // only once per RenderBackend + backend.activate(); + activated = true; + } +} + +void BackendScope::deactivate() { + if (activated && + !(nextScope && this->backend == nextScope->backend)) { + // Only deactivate when set to Explicit and + // only once per RenderBackend + backend.deactivate(); + activated = false; + } +} + bool BackendScope::exists() { return currentScope.get(); } diff --git a/test/renderer/backend_scope.test.cpp b/test/renderer/backend_scope.test.cpp new file mode 100644 index 0000000000..a8c92bb31f --- /dev/null +++ b/test/renderer/backend_scope.test.cpp @@ -0,0 +1,111 @@ +#include + +#include +#include + +#include + +using namespace mbgl; + +class StubRendererBackend: public RendererBackend { +public: + + void activate() override { + if (activateFunction) activateFunction(); + } + + void deactivate() override { + if (deactivateFunction) deactivateFunction(); + } + + void updateAssumedState() override { + if (updateAssumedStateFunction) updateAssumedStateFunction(); + } + + gl::ProcAddress initializeExtension(const char* ext) override { + if (initializeExtensionFunction) { + return initializeExtensionFunction(ext); + } else { + return {}; + } + } + + std::function activateFunction; + std::function deactivateFunction; + std::function updateAssumedStateFunction; + std::function initializeExtensionFunction; +}; + +// A scope should activate on construction +// and deactivate on descruction (going out +// of scope) +TEST(BackendScope, SingleScope) { + bool activated; + bool deactivated; + + StubRendererBackend backend; + backend.activateFunction = [&] { activated = true; }; + backend.deactivateFunction = [&] { deactivated = true; }; + + { + BackendScope test { backend }; + } + + ASSERT_TRUE(activated); + ASSERT_TRUE(deactivated); +} + +// With nested scopes, only the outer scope +// should activate/deactivate +TEST(BackendScope, NestedScopes) { + int activated = 0; + int deactivated = 0; + + StubRendererBackend backend; + backend.activateFunction = [&] { activated++; }; + backend.deactivateFunction = [&] { deactivated++; }; + + { + BackendScope outer { backend }; + ASSERT_EQ(1, activated); + { + BackendScope inner { backend }; + ASSERT_EQ(1, activated); + } + ASSERT_EQ(0, deactivated); + } + + ASSERT_EQ(1, deactivated); +} + +// With chained scopes, where scopes have +// different backends, the scopes should each +// activate the scope on entering and de-activating +// on leaving the scope +TEST(BackendScope, ChainedScopes) { + bool activatedA = false; + bool activatedB = false; + + StubRendererBackend backendA; + backendA.activateFunction = [&] { activatedA = true; }; + backendA.deactivateFunction = [&] { activatedA = false; }; + + StubRendererBackend backendB; + backendB.activateFunction = [&] { activatedB = true; }; + backendB.deactivateFunction = [&] { activatedB = false; }; + + { + BackendScope scopeA { backendA }; + ASSERT_TRUE(activatedA); + { + BackendScope scopeB { backendB }; + ASSERT_FALSE(activatedA); + ASSERT_TRUE(activatedB); + } + ASSERT_FALSE(activatedB); + ASSERT_TRUE(activatedA); + } + + ASSERT_FALSE(activatedA); + ASSERT_FALSE(activatedB); +} -- cgit v1.2.1