From e04a3d7d3a8c252868c2593b535388c22ae3d478 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 1 Jun 2021 10:37:21 -0700 Subject: [PATCH 01/24] AttachmentPoint in java and c++ mismatch fixes #4055 --- .../google/android/filament/RenderTarget.java | 4 ++++ .../backend/include/backend/TargetBufferInfo.h | 2 ++ filament/include/filament/RenderTarget.h | 2 +- filament/src/RenderTarget.cpp | 18 +++++++++--------- samples/rendertarget.cpp | 4 ++-- 5 files changed, 18 insertions(+), 12 deletions(-) diff --git a/android/filament-android/src/main/java/com/google/android/filament/RenderTarget.java b/android/filament-android/src/main/java/com/google/android/filament/RenderTarget.java index 17370a24cbf..0b7dbe0c545 100644 --- a/android/filament-android/src/main/java/com/google/android/filament/RenderTarget.java +++ b/android/filament-android/src/main/java/com/google/android/filament/RenderTarget.java @@ -55,6 +55,10 @@ public enum AttachmentPoint { COLOR1, COLOR2, COLOR3, + COLOR4, + COLOR5, + COLOR6, + COLOR7, DEPTH } diff --git a/filament/backend/include/backend/TargetBufferInfo.h b/filament/backend/include/backend/TargetBufferInfo.h index 0cb445213c6..3c505d065e5 100644 --- a/filament/backend/include/backend/TargetBufferInfo.h +++ b/filament/backend/include/backend/TargetBufferInfo.h @@ -59,6 +59,8 @@ class TargetBufferInfo { class MRT { public: static constexpr uint8_t MIN_SUPPORTED_RENDER_TARGET_COUNT = 4u; + + // When updating this, make sure to also take care of RenderTarget.java static constexpr uint8_t MAX_SUPPORTED_RENDER_TARGET_COUNT = 8u; private: diff --git a/filament/include/filament/RenderTarget.h b/filament/include/filament/RenderTarget.h index ae9f38a1368..cc4ae38a1ce 100644 --- a/filament/include/filament/RenderTarget.h +++ b/filament/include/filament/RenderTarget.h @@ -58,7 +58,7 @@ class UTILS_PUBLIC RenderTarget : public FilamentAPI { /** * Attachment identifiers */ - enum AttachmentPoint { + enum class AttachmentPoint : uint8_t { COLOR0 = 0, //!< identifies the 1st color attachment COLOR1 = 1, //!< identifies the 2nd color attachment COLOR2 = 2, //!< identifies the 3rd color attachment diff --git a/filament/src/RenderTarget.cpp b/filament/src/RenderTarget.cpp index 48cdcdc467f..a4ea9001ad0 100644 --- a/filament/src/RenderTarget.cpp +++ b/filament/src/RenderTarget.cpp @@ -45,29 +45,29 @@ BuilderType::Builder& BuilderType::Builder::operator=(BuilderType::Builder const BuilderType::Builder& BuilderType::Builder::operator=(BuilderType::Builder&& rhs) noexcept = default; RenderTarget::Builder& RenderTarget::Builder::texture(AttachmentPoint pt, Texture* texture) noexcept { - mImpl->mAttachments[pt].texture = upcast(texture); + mImpl->mAttachments[(size_t)pt].texture = upcast(texture); return *this; } RenderTarget::Builder& RenderTarget::Builder::mipLevel(AttachmentPoint pt, uint8_t level) noexcept { - mImpl->mAttachments[pt].mipLevel = level; + mImpl->mAttachments[(size_t)pt].mipLevel = level; return *this; } RenderTarget::Builder& RenderTarget::Builder::face(AttachmentPoint pt, CubemapFace face) noexcept { - mImpl->mAttachments[pt].face = face; + mImpl->mAttachments[(size_t)pt].face = face; return *this; } RenderTarget::Builder& RenderTarget::Builder::layer(AttachmentPoint pt, uint32_t layer) noexcept { - mImpl->mAttachments[pt].layer = layer; + mImpl->mAttachments[(size_t)pt].layer = layer; return *this; } RenderTarget* RenderTarget::Builder::build(Engine& engine) { using backend::TextureUsage; - const FRenderTarget::Attachment& color = mImpl->mAttachments[COLOR0]; - const FRenderTarget::Attachment& depth = mImpl->mAttachments[DEPTH]; + const FRenderTarget::Attachment& color = mImpl->mAttachments[(size_t)AttachmentPoint::COLOR0]; + const FRenderTarget::Attachment& depth = mImpl->mAttachments[(size_t)AttachmentPoint::DEPTH]; if (!ASSERT_PRECONDITION_NON_FATAL(color.texture, "COLOR0 attachment not set")) { return nullptr; } @@ -128,7 +128,7 @@ FRenderTarget::FRenderTarget(FEngine& engine, const RenderTarget::Builder& build TargetBufferInfo dinfo{}; auto setAttachment = [this](TargetBufferInfo& info, AttachmentPoint attachmentPoint) { - Attachment const& attachment = mAttachments[attachmentPoint]; + Attachment const& attachment = mAttachments[(size_t)attachmentPoint]; auto t = upcast(attachment.texture); info.handle = t->getHwHandle(); info.level = attachment.mipLevel; @@ -145,9 +145,9 @@ FRenderTarget::FRenderTarget(FEngine& engine, const RenderTarget::Builder& build setAttachment(mrt[i], (AttachmentPoint)i); } } - if (mAttachments[DEPTH].texture) { + if (mAttachments[(size_t)AttachmentPoint::DEPTH].texture) { mAttachmentMask |= TargetBufferFlags::DEPTH; - setAttachment(dinfo, DEPTH); + setAttachment(dinfo, AttachmentPoint::DEPTH); } FEngine::DriverApi& driver = engine.getDriverApi(); diff --git a/samples/rendertarget.cpp b/samples/rendertarget.cpp index 70ea8156fbb..36af1ff4d2c 100644 --- a/samples/rendertarget.cpp +++ b/samples/rendertarget.cpp @@ -211,8 +211,8 @@ int main(int argc, char** argv) { .usage(Texture::Usage::DEPTH_ATTACHMENT) .format(Texture::InternalFormat::DEPTH24).build(*engine); app.offscreenRenderTarget = RenderTarget::Builder() - .texture(RenderTarget::COLOR, app.offscreenColorTexture) - .texture(RenderTarget::DEPTH, app.offscreenDepthTexture) + .texture(RenderTarget::AttachmentPoint::COLOR, app.offscreenColorTexture) + .texture(RenderTarget::AttachmentPoint::DEPTH, app.offscreenDepthTexture) .build(*engine); app.offscreenView->setRenderTarget(app.offscreenRenderTarget); app.offscreenView->setViewport({0, 0, vp.width, vp.height}); From 6fcc2b579d6451aab1e107dac7af37e6c008c2fe Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 1 Jun 2021 10:49:15 -0700 Subject: [PATCH 02/24] fix LightManager::getFalloff fixes #4043 --- filament/src/components/LightManager.cpp | 2 +- filament/src/components/LightManager.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/filament/src/components/LightManager.cpp b/filament/src/components/LightManager.cpp index 6080b9d8e1a..c0f95dab80f 100644 --- a/filament/src/components/LightManager.cpp +++ b/filament/src/components/LightManager.cpp @@ -479,7 +479,7 @@ void LightManager::setFalloff(Instance i, float radius) noexcept { } float LightManager::getFalloff(Instance i) const noexcept { - return upcast(this)->getSquaredFalloffInv(i); + return upcast(this)->getFalloff(i); } void LightManager::setSpotLightCone(Instance i, float inner, float outer) noexcept { diff --git a/filament/src/components/LightManager.h b/filament/src/components/LightManager.h index fda5064d912..049e4e4d283 100644 --- a/filament/src/components/LightManager.h +++ b/filament/src/components/LightManager.h @@ -187,6 +187,10 @@ class FLightManager : public LightManager { return mManager[i].squaredFallOffInv; } + float getFalloff(Instance i) const noexcept { + return getRadius(i); + } + SpotParams const& getSpotParams(Instance i) const noexcept { return mManager[i].spotParams; } From 9a83805f19629fc8b1362170fcf77d6bbfbae2d4 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Mon, 24 May 2021 10:22:55 -0700 Subject: [PATCH 03/24] build.sh -C option entirely cleans the android/ directory All (and only) git-ignored files under android/ are deleted. This is to prevent a recurring problem where "clean" builds were not actually clean in some cases. --- build.sh | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/build.sh b/build.sh index 015a7dd5b02..b00f73d8640 100755 --- a/build.sh +++ b/build.sh @@ -17,6 +17,10 @@ function print_help { echo " Generate .tgz build archives, implies -i." echo " -c" echo " Clean build directories." + echo " -C" + echo " Clean build directories and revert android/ to a freshly sync'ed state." + echo " All (and only) git-ignored files under android/ are deleted." + echo " This is sometimes needed instead of -c (which still misses some clean steps)." echo " -d" echo " Enable matdbg and disable material optimization." echo " -f" @@ -112,6 +116,7 @@ CMAKE_MINOR=19 # Internal variables ISSUE_CLEAN=false +ISSUE_CLEAN_AGGRESSIVE=false ISSUE_DEBUG_BUILD=false ISSUE_RELEASE_BUILD=false @@ -170,6 +175,12 @@ function build_clean { rm -Rf android/filament-utils-android/build android/filament-utils-android/.externalNativeBuild android/filament-utils-android/.cxx } +function build_clean_aggressive { + echo "Cleaning build directories..." + rm -Rf out + git clean -qfX android +} + function build_desktop_target { local lc_target=$(echo "$1" | tr '[:upper:]' '[:lower:]') local build_targets=$2 @@ -678,7 +689,7 @@ function run_tests { pushd "$(dirname "$0")" > /dev/null -while getopts ":hacfijmp:q:uvslwtd" opt; do +while getopts ":hacCfijmp:q:uvslwtd" opt; do case ${opt} in h) print_help @@ -691,6 +702,9 @@ while getopts ":hacfijmp:q:uvslwtd" opt; do c) ISSUE_CLEAN=true ;; + C) + ISSUE_CLEAN_AGGRESSIVE=true + ;; d) PRINT_MATDBG_HELP=true MATDBG_OPTION="-DFILAMENT_ENABLE_MATDBG=ON, -DFILAMENT_DISABLE_MATOPT=ON" @@ -830,6 +844,10 @@ if [[ "${ISSUE_CLEAN}" == "true" ]]; then build_clean fi +if [[ "${ISSUE_CLEAN_AGGRESSIVE}" == "true" ]]; then + build_clean_aggressive +fi + if [[ "${ISSUE_DESKTOP_BUILD}" == "true" ]]; then build_desktop fi From 4d3f10477a27214dfe5b96dcecfc15164eeeedab Mon Sep 17 00:00:00 2001 From: James Walker Date: Tue, 1 Jun 2021 16:35:23 -0700 Subject: [PATCH 04/24] fix macOS main thread checker warnings with OpenGL (#4011) * fix runtime warnings about certain NSView methods being called off the main thread * address code style issues * fix code style and threading concerns * Remove unneeded __strong qualifiers, put __weak after the star as recommended by Apple, convert weak to strong before testing for nil to avoid a race * convert tabs to spaces --- .../backend/src/opengl/PlatformCocoaGL.mm | 97 +++++++++++++++++-- 1 file changed, 87 insertions(+), 10 deletions(-) diff --git a/filament/backend/src/opengl/PlatformCocoaGL.mm b/filament/backend/src/opengl/PlatformCocoaGL.mm index 50939e57b74..01a2dd95d99 100644 --- a/filament/backend/src/opengl/PlatformCocoaGL.mm +++ b/filament/backend/src/opengl/PlatformCocoaGL.mm @@ -35,9 +35,15 @@ using namespace backend; struct CocoaGLSwapChain : public Platform::SwapChain { + CocoaGLSwapChain(NSView* inView); + ~CocoaGLSwapChain() noexcept; + NSView* view; NSRect previousBounds; NSRect previousWindowFrame; + NSMutableArray>* observers; + NSRect currentBounds; + NSRect currentWindowFrame; }; struct PlatformCocoaGLImpl { @@ -47,6 +53,83 @@ void updateOpenGLContext(NSView *nsView, bool resetView, bool clearView); }; +CocoaGLSwapChain::CocoaGLSwapChain( NSView* inView ) + : view(inView) + , previousBounds(NSZeroRect) + , previousWindowFrame(NSZeroRect) + , observers([NSMutableArray array]) + , currentBounds(NSZeroRect) + , currentWindowFrame(NSZeroRect) { + NSView* __weak weakView = view; + NSMutableArray* __weak weakObservers = observers; + + void (^notificationHandler)(NSNotification *notification) = ^(NSNotification *notification) { + NSView* strongView = weakView; + if ((weakView != nil) && (weakObservers != nil)) { + this->currentBounds = [strongView convertRectToBacking: strongView.bounds]; + this->currentWindowFrame = strongView.window.frame; + } + }; + + // Various methods below should only be called from the main thread: + // -[NSView bounds], -[NSView convertRectToBacking:], -[NSView window], + // -[NSWindow frame], -[NSView superview], + // -[NSView setPostsFrameChangedNotifications:], + // -[NSView setPostsBoundsChangedNotifications:] + dispatch_async(dispatch_get_main_queue(), ^(void) { + NSView* strongView = weakView; + NSMutableArray* strongObservers = weakObservers; + if ((weakView == nil) || (weakObservers == nil)) { + return; + } + @synchronized (strongObservers) { + this->currentBounds = [strongView convertRectToBacking: strongView.bounds]; + this->currentWindowFrame = strongView.window.frame; + + id observer = [NSNotificationCenter.defaultCenter + addObserverForName: NSWindowDidResizeNotification + object: strongView.window + queue: nil + usingBlock: notificationHandler]; + [strongObservers addObject: observer]; + observer = [NSNotificationCenter.defaultCenter + addObserverForName: NSWindowDidMoveNotification + object: strongView.window + queue: nil + usingBlock: notificationHandler]; + [strongObservers addObject: observer]; + + NSView* aView = strongView; + while (aView != nil) { + aView.postsFrameChangedNotifications = YES; + aView.postsBoundsChangedNotifications = YES; + observer = [NSNotificationCenter.defaultCenter + addObserverForName: NSViewFrameDidChangeNotification + object: aView + queue: nil + usingBlock: notificationHandler]; + [strongObservers addObject: observer]; + observer = [NSNotificationCenter.defaultCenter + addObserverForName: NSViewBoundsDidChangeNotification + object: aView + queue: nil + usingBlock: notificationHandler]; + [strongObservers addObject: observer]; + + aView = aView.superview; + } + } + }); +} + +CocoaGLSwapChain::~CocoaGLSwapChain() noexcept { + @synchronized (observers) { + for (id observer in observers) { + [NSNotificationCenter.defaultCenter removeObserver: observer]; + } + } +} + PlatformCocoaGL::PlatformCocoaGL() : pImpl(new PlatformCocoaGLImpl) { } @@ -91,10 +174,7 @@ flags &= ~backend::SWAP_CHAIN_CONFIG_TRANSPARENT; NSView* nsView = (__bridge NSView*)nativewindow; - CocoaGLSwapChain* swapChain = new CocoaGLSwapChain(); - swapChain->view = nsView; - swapChain->previousBounds = NSZeroRect; - swapChain->previousWindowFrame = NSZeroRect; + CocoaGLSwapChain* swapChain = new CocoaGLSwapChain( nsView ); // If the SwapChain is being recreated (e.g. if the underlying surface has been resized), // then we need to force an update to occur in the subsequent makeCurrent, which can be done by @@ -112,10 +192,7 @@ // adding the pointer to the array retains the NSView pImpl->mHeadlessSwapChains.push_back(nsView); - CocoaGLSwapChain* swapChain = new CocoaGLSwapChain(); - swapChain->view = nsView; - swapChain->previousBounds = NSZeroRect; - swapChain->previousWindowFrame = NSZeroRect; + CocoaGLSwapChain* swapChain = new CocoaGLSwapChain( nsView ); return swapChain; } @@ -140,8 +217,8 @@ ASSERT_PRECONDITION_NON_FATAL(drawSwapChain == readSwapChain, "ContextManagerCocoa does not support using distinct draw/read swap chains."); CocoaGLSwapChain* swapChain = (CocoaGLSwapChain*)drawSwapChain; - NSRect currentBounds = [swapChain->view convertRectToBacking:swapChain->view.bounds]; - NSRect currentWindowFrame = swapChain->view.window.frame; + NSRect currentBounds = swapChain->currentBounds; + NSRect currentWindowFrame = swapChain->currentWindowFrame; // Check if the view has been swapped out or resized. // updateOpenGLContext() needs to call -clearDrawable if the view was From eafbf699a2684190576fd2d59319c46c370aee3e Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Tue, 1 Jun 2021 23:39:38 -0700 Subject: [PATCH 05/24] Turn off warnings as errors for spirv-tools --- third_party/spirv-tools/CMakeLists.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/third_party/spirv-tools/CMakeLists.txt b/third_party/spirv-tools/CMakeLists.txt index 9ea29ab2eaf..41ae7897cab 100755 --- a/third_party/spirv-tools/CMakeLists.txt +++ b/third_party/spirv-tools/CMakeLists.txt @@ -94,7 +94,10 @@ endif(SPIRV_BUILD_COMPRESSION) option(SPIRV_BUILD_FUZZER "Build spirv-fuzz" OFF) -option(SPIRV_WERROR "Enable error on warning" ON) +# Filament specific changes +# option(SPIRV_WERROR "Enable error on warning" ON) +set(SPIRV_WERROR OFF) +# End Filament specific changes if(("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU") OR (("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") AND (NOT CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC"))) set(COMPILER_IS_LIKE_GNU TRUE) endif() From e0a8284fcfd15a5ccb681e002ba8e3e2843abfe1 Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Tue, 1 Jun 2021 23:31:36 -0700 Subject: [PATCH 06/24] Update filament-specific-changes.patch --- .../filament-specific-changes.patch | 79 +++++++++++++------ 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/third_party/spirv-tools/filament-specific-changes.patch b/third_party/spirv-tools/filament-specific-changes.patch index e1c6d588129..55e95faff26 100644 --- a/third_party/spirv-tools/filament-specific-changes.patch +++ b/third_party/spirv-tools/filament-specific-changes.patch @@ -1,5 +1,17 @@ +From f2c7730fd3246c70c6cb23f1637aa05a3a8b517a Mon Sep 17 00:00:00 2001 +From: Benjamin Doherty +Date: Tue, 1 Jun 2021 23:31:16 -0700 +Subject: [PATCH] Filament specific changes + +--- + third_party/spirv-tools/CMakeLists.txt | 86 ++++++++++++------- + .../spirv-tools/external/CMakeLists.txt | 6 +- + .../external/spirv-headers/CMakeLists.txt | 8 +- + third_party/spirv-tools/source/CMakeLists.txt | 26 ++++-- + 4 files changed, 81 insertions(+), 45 deletions(-) + diff --git a/third_party/spirv-tools/CMakeLists.txt b/third_party/spirv-tools/CMakeLists.txt -index 6ed56a81..2392dfd8 100755 +index 55f84e6d8..41ae7897c 100755 --- a/third_party/spirv-tools/CMakeLists.txt +++ b/third_party/spirv-tools/CMakeLists.txt @@ -24,8 +24,19 @@ if (POLICY CMP0054) @@ -34,7 +46,32 @@ index 6ed56a81..2392dfd8 100755 if(NOT ${SKIP_SPIRV_TOOLS_INSTALL}) set(ENABLE_SPIRV_TOOLS_INSTALL ON) endif() -@@ -231,11 +244,13 @@ if(ENABLE_SPIRV_TOOLS_INSTALL) +@@ -81,7 +94,10 @@ endif(SPIRV_BUILD_COMPRESSION) + + option(SPIRV_BUILD_FUZZER "Build spirv-fuzz" OFF) + +-option(SPIRV_WERROR "Enable error on warning" ON) ++# Filament specific changes ++# option(SPIRV_WERROR "Enable error on warning" ON) ++set(SPIRV_WERROR OFF) ++# End Filament specific changes + if(("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU") OR (("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") AND (NOT CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC"))) + set(COMPILER_IS_LIKE_GNU TRUE) + endif() +@@ -230,7 +246,11 @@ if(NOT COMMAND find_host_program) + endif() + + # Tests require Python3 +-find_host_package(PythonInterp 3 REQUIRED) ++if(CMAKE_VERSION VERSION_LESS "3.12" OR ${CMAKE_SYSTEM_NAME} MATCHES "Windows") ++ find_host_package(PythonInterp 3 REQUIRED) ++else() ++ find_package(Python3 COMPONENTS Interpreter) ++endif() + + # Check for symbol exports on Linux. + # At the moment, this check will fail on the OSX build machines for the Android NDK. +@@ -273,11 +293,13 @@ if(ENABLE_SPIRV_TOOLS_INSTALL) endif() # Defaults to OFF if the user didn't set it. @@ -50,7 +87,7 @@ index 6ed56a81..2392dfd8 100755 if ("${SPIRV_SKIP_EXECUTABLES}") set(SPIRV_SKIP_TESTS ON) endif() -@@ -279,8 +294,10 @@ endif() +@@ -321,8 +343,10 @@ endif() add_subdirectory(source) add_subdirectory(tools) @@ -63,7 +100,7 @@ index 6ed56a81..2392dfd8 100755 if(ENABLE_SPIRV_TOOLS_INSTALL) install( -@@ -305,28 +322,30 @@ set(SPIRV_SHARED_LIBRARIES "-lSPIRV-Tools-shared") +@@ -347,28 +371,30 @@ set(SPIRV_SHARED_LIBRARIES "-lSPIRV-Tools-shared") # Build pkg-config file # Use a first-class target so it's regenerated when relevant files are updated. @@ -116,19 +153,6 @@ index 6ed56a81..2392dfd8 100755 # Install pkg-config file if (ENABLE_SPIRV_TOOLS_INSTALL) -@@ -201,7 +201,11 @@ if(NOT COMMAND find_host_program) - endif() - - # Tests require Python3 --find_host_package(PythonInterp 3 REQUIRED) -+if(CMAKE_VERSION VERSION_LESS "3.12" OR ${CMAKE_SYSTEM_NAME} MATCHES "Windows") -+ find_host_package(PythonInterp 3 REQUIRED) -+else() -+ find_package(Python3 COMPONENTS Interpreter) -+endif() - - # Check for symbol exports on Linux. - # At the moment, this check will fail on the OSX build machines for the Android NDK. diff --git a/third_party/spirv-tools/external/CMakeLists.txt b/third_party/spirv-tools/external/CMakeLists.txt index 179a4012f..152cfc421 100644 --- a/third_party/spirv-tools/external/CMakeLists.txt @@ -147,31 +171,31 @@ index 179a4012f..152cfc421 100644 endif() else() diff --git a/third_party/spirv-tools/external/spirv-headers/CMakeLists.txt b/third_party/spirv-tools/external/spirv-headers/CMakeLists.txt -index eb4694786..d3940532d 100644 +index 6f01ef098..ef2e56606 100644 --- a/third_party/spirv-tools/external/spirv-headers/CMakeLists.txt +++ b/third_party/spirv-tools/external/spirv-headers/CMakeLists.txt @@ -49,11 +49,11 @@ add_custom_target(install-headers COMMAND cmake -E copy_directory ${CMAKE_CURRENT_SOURCE_DIR}/include/spirv $ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/include/spirv) - + -option(SPIRV_HEADERS_SKIP_EXAMPLES "Skip building examples" - ${SPIRV_HEADERS_SKIP_EXAMPLES}) +# Filament specific changes +option(SPIRV_HEADERS_SKIP_EXAMPLES "Skip building examples" ON) - + -option(SPIRV_HEADERS_SKIP_INSTALL "Skip install" - ${SPIRV_HEADERS_SKIP_INSTALL}) +option(SPIRV_HEADERS_SKIP_INSTALL "Skip install" ON) +# End Filament specific changes - + if(NOT ${SPIRV_HEADERS_SKIP_EXAMPLES}) set(SPIRV_HEADERS_ENABLE_EXAMPLES ON) diff --git a/third_party/spirv-tools/source/CMakeLists.txt b/third_party/spirv-tools/source/CMakeLists.txt -index 65087f2c..38101ab2 100644 +index 6633bc916..5f5904567 100644 --- a/third_party/spirv-tools/source/CMakeLists.txt +++ b/third_party/spirv-tools/source/CMakeLists.txt @@ -364,13 +364,15 @@ endfunction() - + # Always build ${SPIRV_TOOLS}-shared. This is expected distro packages, and # unlike the other SPIRV_TOOLS target, defaults to hidden symbol visibility. -add_library(${SPIRV_TOOLS}-shared SHARED ${SPIRV_SOURCES}) @@ -190,13 +214,13 @@ index 65087f2c..38101ab2 100644 +# PUBLIC SPIRV_TOOLS_SHAREDLIB +# ) +# End Filament specific changes - + if(SPIRV_TOOLS_BUILD_STATIC) add_library(${SPIRV_TOOLS}-static STATIC ${SPIRV_SOURCES}) @@ -386,11 +388,17 @@ if(SPIRV_TOOLS_BUILD_STATIC) add_library(${SPIRV_TOOLS} ALIAS ${SPIRV_TOOLS}-static) endif() - + - set(SPIRV_TOOLS_TARGETS ${SPIRV_TOOLS}-static ${SPIRV_TOOLS}-shared) +# Filament specific changes + # set(SPIRV_TOOLS_TARGETS ${SPIRV_TOOLS}-static ${SPIRV_TOOLS}-shared) @@ -211,5 +235,8 @@ index 65087f2c..38101ab2 100644 + set(SPIRV_TOOLS_TARGETS ${SPIRV_TOOLS}) +# End Filament specific changes endif() - + if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") +-- +2.32.0.rc0.204.g9fa02ecfa5-goog + From 384d6921f642ee1dfc9d4136607b3cfae5dc2f07 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 2 Jun 2021 11:46:10 -0700 Subject: [PATCH 07/24] add system property to override the backend selection On Android, set `debug.filament.backend` to the desired backend: 0: default 1: gl/gles 2: vulkan 3: metal 4: noop example: `adb shell setprop debug.filament.backend 2` This will affect all filament apps on the device. This sets the backed as if it was set from the public API. --- filament/backend/src/Platform.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/filament/backend/src/Platform.cpp b/filament/backend/src/Platform.cpp index 6e96cbb0f82..454e4792a3b 100644 --- a/filament/backend/src/Platform.cpp +++ b/filament/backend/src/Platform.cpp @@ -20,6 +20,7 @@ #include #if defined(ANDROID) + #include #if defined(FILAMENT_SUPPORTS_OPENGL) && !defined(FILAMENT_USE_EXTERNAL_GLES3) #include "opengl/PlatformEGLAndroid.h" #endif @@ -82,6 +83,15 @@ Platform::~Platform() noexcept = default; DefaultPlatform* DefaultPlatform::create(Backend* backend) noexcept { SYSTRACE_CALL(); assert_invariant(backend); + +#if defined(ANDROID) + char scratch[PROP_VALUE_MAX + 1]; + int length = __system_property_get("debug.filament.backend", scratch); + if (length > 0) { + *backend = Backend(atoi(scratch)); + } +#endif + if (*backend == Backend::DEFAULT) { *backend = Backend::OPENGL; } From 4c9ae9dd91a41c4b0d3c2f3486e971a4ba818b37 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 1 Jun 2021 17:34:38 -0700 Subject: [PATCH 08/24] use c++ thread_local now that it's supported on Android there are cases where thread_local still doesn't work well, but we're not using those cases. --- .../android/ExternalStreamManagerAndroid.cpp | 2 +- .../backend/src/android/VirtualMachineEnv.h | 4 +- libs/utils/CMakeLists.txt | 1 - libs/utils/include/utils/Log.h | 1 - libs/utils/include/utils/ThreadLocal.h | 199 ------------------ libs/utils/include/utils/compiler.h | 9 +- 6 files changed, 4 insertions(+), 212 deletions(-) delete mode 100644 libs/utils/include/utils/ThreadLocal.h diff --git a/filament/backend/src/android/ExternalStreamManagerAndroid.cpp b/filament/backend/src/android/ExternalStreamManagerAndroid.cpp index c333c947715..1f33f34bc20 100644 --- a/filament/backend/src/android/ExternalStreamManagerAndroid.cpp +++ b/filament/backend/src/android/ExternalStreamManagerAndroid.cpp @@ -39,7 +39,7 @@ static void loadSymbol(T*& pfn, const char *symbol) noexcept { ExternalStreamManagerAndroid& ExternalStreamManagerAndroid::get() noexcept { // declaring this thread local, will ensure it's destroyed with the calling thread - static UTILS_DECLARE_TLS(ExternalStreamManagerAndroid) instance; + static thread_local ExternalStreamManagerAndroid instance; return instance; } diff --git a/filament/backend/src/android/VirtualMachineEnv.h b/filament/backend/src/android/VirtualMachineEnv.h index 5534129eb9d..eb198def944 100644 --- a/filament/backend/src/android/VirtualMachineEnv.h +++ b/filament/backend/src/android/VirtualMachineEnv.h @@ -18,7 +18,6 @@ #define TNT_FILAMENT_DRIVER_ANDROID_VIRTUAL_MACHINE_ENV_H #include -#include #include #include @@ -31,8 +30,7 @@ class VirtualMachineEnv { static VirtualMachineEnv& get() noexcept { // declaring this thread local, will ensure it's destroyed with the calling thread - static UTILS_DECLARE_TLS(VirtualMachineEnv) - instance; + static thread_local VirtualMachineEnv instance; return instance; } diff --git a/libs/utils/CMakeLists.txt b/libs/utils/CMakeLists.txt index 4a3ffea3801..bc3e9a94909 100644 --- a/libs/utils/CMakeLists.txt +++ b/libs/utils/CMakeLists.txt @@ -33,7 +33,6 @@ set(DIST_HDRS ${PUBLIC_HDR_DIR}/${TARGET}/Slice.h ${PUBLIC_HDR_DIR}/${TARGET}/SpinLock.h ${PUBLIC_HDR_DIR}/${TARGET}/StructureOfArrays.h - ${PUBLIC_HDR_DIR}/${TARGET}/ThreadLocal.h ${PUBLIC_HDR_DIR}/${TARGET}/unwindows.h ) diff --git a/libs/utils/include/utils/Log.h b/libs/utils/include/utils/Log.h index 08f9ef1093a..f53287c652f 100644 --- a/libs/utils/include/utils/Log.h +++ b/libs/utils/include/utils/Log.h @@ -19,7 +19,6 @@ #include -#include #include #include // ssize_t is a POSIX type. diff --git a/libs/utils/include/utils/ThreadLocal.h b/libs/utils/include/utils/ThreadLocal.h deleted file mode 100644 index 6301a0dcd16..00000000000 --- a/libs/utils/include/utils/ThreadLocal.h +++ /dev/null @@ -1,199 +0,0 @@ -/* - * Copyright (C) 2016 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef TNT_UTILS_THREADLOCAL_H -#define TNT_UTILS_THREADLOCAL_H - -#include - -#if UTILS_HAS_FEATURE_CXX_THREAD_LOCAL - -#define UTILS_DECLARE_TLS(clazz) thread_local clazz -#define UTILS_DEFINE_TLS(clazz) thread_local clazz - -#else // UTILS_HAS_FEATURE_CXX_THREAD_LOCAL - -#define UTILS_DECLARE_TLS(clazz) utils::ThreadLocal -#define UTILS_DEFINE_TLS(clazz) utils::ThreadLocal - -#include -#include - -namespace utils { - -/* ------------------------------------------------------------------------------------------------ - * ThreadLocal is a workaround for compilers/platforms that don't support C++0x11 thread_local - * e.g.: - * static ThreadLocal s_current_thread(args); - * - * is equivalent to: - * - * static thread_local Foo s_current_thread(args); - * - */ - -// TODO: Specialization for basic types (to avoid heap-allocation) - -template -class ThreadLocal { -public: - ThreadLocal() noexcept { - pthread_key_create(&mKey, destructor); - } - - // construct from the arguments of T ctor - template - explicit ThreadLocal(ARGS&&... args) noexcept : ThreadLocal() { - T* ptr = new T(std::forward(args)...); - pthread_setspecific(mKey, ptr); - } - - ~ThreadLocal() { - pthread_key_delete(mKey); - } - - ThreadLocal(const ThreadLocal& rhs) = delete; - ThreadLocal(ThreadLocal&& rhs) = delete; - ThreadLocal& operator=(const ThreadLocal& rhs) = delete; - ThreadLocal& operator=(ThreadLocal&& rhs) = delete; - - // assign a T - T& operator=(T value) noexcept { - T& r = getRef(); - std::swap(r, value); - return r; - } - - // convert to a T - operator T&() noexcept { - T& r = getRef(); - return r; - } - - operator T const&() const noexcept { - T const& r = const_cast(this)->getRef(); - return r; - } - - // pointer-like access, in case T is a smart pointer for instance - T* operator->() noexcept { - T& r = getRef(); - return &r; - } - - T const* operator->() const noexcept { - T const& r = const_cast(this)->getRef(); - return &r; - } - -private: - // destructor called at thread exit - static void destructor(void *specific) { - T* ptr = static_cast(specific); - delete ptr; - } - - UTILS_NOINLINE - T& getRef() noexcept { - T* ptr = static_cast(pthread_getspecific(mKey)); - if (UTILS_LIKELY(ptr)) { - return *ptr; - } - return allocRef(); - } - - UTILS_NOINLINE - T& allocRef() noexcept { - T* ptr = new T(); - pthread_setspecific(mKey, ptr); - return *ptr; - } - - pthread_key_t mKey; -}; - - -/* - * Specialization for pointers - */ - -template -class ThreadLocal { -public: - ThreadLocal() noexcept { - pthread_key_create(&mKey, nullptr); - } - - // construct from the arguments of T ctor - explicit ThreadLocal(T* rhs) noexcept : ThreadLocal() { - pthread_setspecific(mKey, rhs); - } - - ~ThreadLocal() { - pthread_key_delete(mKey); - } - - ThreadLocal(const ThreadLocal& rhs) = delete; - ThreadLocal(ThreadLocal&& rhs) = delete; - ThreadLocal& operator=(const ThreadLocal& rhs) = delete; - ThreadLocal& operator=(ThreadLocal&& rhs) = delete; - - - ThreadLocal& operator = (T const * rhs) noexcept { - pthread_setspecific(mKey, rhs); - return *this; - } - - - operator bool() noexcept { - return pthread_getspecific(mKey) != nullptr; - } - - // convert to a T* - operator T*() noexcept { - return static_cast(pthread_getspecific(mKey)); - } - - operator T const*() const noexcept { - return static_cast(pthread_getspecific(mKey)); - } - - // access like a pointer - T* operator->() noexcept { - return static_cast(pthread_getspecific(mKey)); - } - - T const* operator->() const noexcept { - return static_cast(pthread_getspecific(mKey)); - } - - T& operator*() noexcept { - return *static_cast(pthread_getspecific(mKey)); - } - - T const& operator*() const noexcept { - return *static_cast(pthread_getspecific(mKey)); - } - -private: - pthread_key_t mKey; -}; - -} // namespace utils - -#endif // UTILS_HAS_FEATURE_CXX_THREAD_LOCAL - -#endif // TNT_UTILS_THREADLOCAL_H diff --git a/libs/utils/include/utils/compiler.h b/libs/utils/include/utils/compiler.h index c0bdebfbcf5..28e6cf963ce 100644 --- a/libs/utils/include/utils/compiler.h +++ b/libs/utils/include/utils/compiler.h @@ -158,14 +158,9 @@ #endif #if defined(_MSC_VER) && _MSC_VER >= 1900 -# define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 1 +# define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 1 #elif __has_feature(cxx_thread_local) -# ifdef ANDROID -# // Android NDK lies about supporting cxx_thread_local -# define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 0 -# else // ANDROID -# define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 1 -# endif // ANDROID +# define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 1 #else # define UTILS_HAS_FEATURE_CXX_THREAD_LOCAL 0 #endif From 9a24c4001f6deaed870f3ce0167dedcf2a468a4a Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 26 May 2021 18:52:24 -0700 Subject: [PATCH 09/24] Flush the CommandStream in a few strategic places The CommandStream hold backend commands, but it has a limited size. With some large models, we can run out of space especially during initialization. We can combat this by flushing the command stream regularly. With this change, we do this every 128 renderable creation. We also flush the CommandStream just before generating the draw commands for the frame. If the buffer is not sized correctly (currently at compile time), this could cause stalls, which will be logged in DEBUG builds. --- filament/src/RenderPass.cpp | 7 ++++++- filament/src/components/RenderableManager.cpp | 1 + filament/src/details/Engine.h | 12 ++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/filament/src/RenderPass.cpp b/filament/src/RenderPass.cpp index 057920a229a..ff2eb928742 100644 --- a/filament/src/RenderPass.cpp +++ b/filament/src/RenderPass.cpp @@ -183,7 +183,12 @@ void RenderPass::execute(const char* name, } void RenderPass::executeCommands(const char* name) const noexcept { - DriverApi& driver = mEngine.getDriverApi(); + // this is a good time to flush the CommandStream, because we're about to potentially + // output a lot of commands. This guarantees here that we have at least + // FILAMENT_MIN_COMMAND_BUFFERS_SIZE_IN_MB bytes (1MiB by default). + FEngine& engine = mEngine; + engine.flush(); + DriverApi& driver = engine.getDriverApi(); RenderPass::recordDriverCommands(driver, mCommands.begin(), mCommands.end()); } diff --git a/filament/src/components/RenderableManager.cpp b/filament/src/components/RenderableManager.cpp index f971caf121e..d5d49e6a235 100644 --- a/filament/src/components/RenderableManager.cpp +++ b/filament/src/components/RenderableManager.cpp @@ -323,6 +323,7 @@ void FRenderableManager::create( } } } + engine.flushIfNeeded(); } // this destroys a single component from an entity diff --git a/filament/src/details/Engine.h b/filament/src/details/Engine.h index 190205be939..483887307c7 100644 --- a/filament/src/details/Engine.h +++ b/filament/src/details/Engine.h @@ -282,6 +282,17 @@ class FEngine : public Engine { // flush the current buffer void flush(); + // flush the current buffer based on some heuristics + void flushIfNeeded() { + auto counter = mFlushCounter + 1; + if (UTILS_LIKELY(counter < 128)) { + mFlushCounter = counter; + } else { + mFlushCounter = 0; + flush(); + } + } + /** * Processes the platform's event queue when called from the platform's event-handling thread. * Returns false when called from any other thread. @@ -375,6 +386,7 @@ class FEngine : public Engine { std::thread mDriverThread; backend::CommandBufferQueue mCommandBufferQueue; DriverApi mCommandStream; + uint32_t mFlushCounter = 0; LinearAllocatorArena mPerRenderPassAllocator; HeapAllocatorArena mHeapAllocator; From 4d73583f8631189dc629ac580e9ddd8351ff36d9 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 1 Jun 2021 23:09:10 -0700 Subject: [PATCH 10/24] Better systrace logging and small improvements to JobSystem Tweak how we signal/wakeup threads so we get better parallelism on Android. --- filament/src/Scene.cpp | 3 +++ filament/src/ShadowMap.cpp | 3 +++ filament/src/View.cpp | 1 + libs/utils/include/utils/JobSystem.h | 1 + libs/utils/src/JobSystem.cpp | 7 ++++++- 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/filament/src/Scene.cpp b/filament/src/Scene.cpp index 541af9a74e5..a9337bc3f36 100644 --- a/filament/src/Scene.cpp +++ b/filament/src/Scene.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -50,6 +51,8 @@ void FScene::prepare(const mat4f& worldOriginTransform) { // TODO: can we skip this in most cases? Since we rely on indices staying the same, // we could only skip, if nothing changed in the RCM. + SYSTRACE_CALL(); + FEngine& engine = mEngine; EntityManager& em = engine.getEntityManager(); FRenderableManager& rcm = engine.getRenderableManager(); diff --git a/filament/src/ShadowMap.cpp b/filament/src/ShadowMap.cpp index 36e1f9f6668..36594dd7afd 100644 --- a/filament/src/ShadowMap.cpp +++ b/filament/src/ShadowMap.cpp @@ -27,6 +27,7 @@ #include #include +#include #include @@ -1061,6 +1062,8 @@ float ShadowMap::texelSizeWorldSpace(const mat4f& Wp, const mat4f& MbMtF) const template void ShadowMap::visitScene(const FScene& scene, uint32_t visibleLayers, Casters casters, Receivers receivers) noexcept { + SYSTRACE_CALL(); + using State = FRenderableManager::Visibility; FScene::RenderableSoa const& UTILS_RESTRICT soa = scene.getRenderableData(); float3 const* const UTILS_RESTRICT worldAABBCenter = soa.data(); diff --git a/filament/src/View.cpp b/filament/src/View.cpp index 473193a4b04..016c306ea5a 100644 --- a/filament/src/View.cpp +++ b/filament/src/View.cpp @@ -743,6 +743,7 @@ void FView::prepareVisibleRenderables(JobSystem& js, void FView::cullRenderables(JobSystem& js, FScene::RenderableSoa& renderableData, Frustum const& frustum, size_t bit) noexcept { + SYSTRACE_CALL(); float3 const* worldAABBCenter = renderableData.data(); float3 const* worldAABBExtent = renderableData.data(); diff --git a/libs/utils/include/utils/JobSystem.h b/libs/utils/include/utils/JobSystem.h index 31382bad6ff..241d69922c8 100644 --- a/libs/utils/include/utils/JobSystem.h +++ b/libs/utils/include/utils/JobSystem.h @@ -470,6 +470,7 @@ struct ParallelForJobData { } void parallelWithJobs(JobSystem& js, JobSystem::Job* parent) noexcept { + assert(parent); // We first split about the number of threads we have, and only then we split the rest // in a single thread (but execute the final cut in new jobs, see parallel() below), diff --git a/libs/utils/src/JobSystem.cpp b/libs/utils/src/JobSystem.cpp index fc2b8cfe482..46c719b6b26 100644 --- a/libs/utils/src/JobSystem.cpp +++ b/libs/utils/src/JobSystem.cpp @@ -253,8 +253,13 @@ void JobSystem::wake() noexcept { lock.lock(); // this empty critical section is needed -- it guarantees that notifiy_all() happens // after the condition variables are set. - lock.unlock(); + + // We signal the condition inside the lock (which is not required), because this seems to + // yield to better scheduling on Android. When we signal outside the critical section, + // it looks like this thread gives its time slice to the waking thread and just sits there + // being runnable, but not running. mWaiterCondition.notify_all(); + lock.unlock(); } inline JobSystem::ThreadState& JobSystem::getState() noexcept { From fb2a95470aba38dda5d46c84287d27f9b56e19b0 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Tue, 1 Jun 2021 14:11:12 -0700 Subject: [PATCH 11/24] bring some improvements to logging from a reverted change - cleanup Log header file - lazy initialization of the log buffer - aggressively resize down the log buffer --- filament/backend/src/opengl/OpenGLContext.cpp | 4 +- libs/utils/include/utils/Log.h | 34 +++------------- libs/utils/include/utils/ostream.h | 10 ++--- libs/utils/src/Log.cpp | 17 +++++++- libs/utils/src/ostream.cpp | 40 ++++++++++--------- 5 files changed, 50 insertions(+), 55 deletions(-) diff --git a/filament/backend/src/opengl/OpenGLContext.cpp b/filament/backend/src/opengl/OpenGLContext.cpp index 62aa98f8017..0bbb5b3b6a5 100644 --- a/filament/backend/src/opengl/OpenGLContext.cpp +++ b/filament/backend/src/opengl/OpenGLContext.cpp @@ -180,7 +180,7 @@ OpenGLContext::OpenGLContext() noexcept { if (ext.KHR_debug) { auto cb = [](GLenum source, GLenum type, GLuint id, GLenum severity, GLsizei length, const GLchar* message, const void *userParam) { - io::LogStream* stream = nullptr; + io::ostream* stream = nullptr; switch (severity) { case GL_DEBUG_SEVERITY_HIGH: stream = &slog.e; @@ -196,7 +196,7 @@ OpenGLContext::OpenGLContext() noexcept { stream = &slog.i; break; } - io::LogStream& out = *stream; + io::ostream& out = *stream; out << "KHR_debug "; switch (type) { case GL_DEBUG_TYPE_ERROR: diff --git a/libs/utils/include/utils/Log.h b/libs/utils/include/utils/Log.h index f53287c652f..dcf7a1cbe47 100644 --- a/libs/utils/include/utils/Log.h +++ b/libs/utils/include/utils/Log.h @@ -17,48 +17,26 @@ #ifndef TNT_UTILS_LOG_H #define TNT_UTILS_LOG_H -#include - -#include -#include // ssize_t is a POSIX type. - +#include #include namespace utils { -namespace io { - -class UTILS_PUBLIC LogStream : public ostream { -public: - - enum Priority { - LOG_DEBUG, LOG_ERROR, LOG_WARNING, LOG_INFO - }; - - explicit LogStream(Priority p) noexcept : mPriority(p) {} - - ostream& flush() noexcept override; - -private: - Priority mPriority; -}; - -} // namespace io struct UTILS_PUBLIC Loggers { // DEBUG level logging stream - io::LogStream& d; + io::ostream& d; // ERROR level logging stream - io::LogStream& e; + io::ostream& e; // WARNING level logging stream - io::LogStream& w; + io::ostream& w; // INFORMATION level logging stream - io::LogStream& i; + io::ostream& i; }; -extern UTILS_PUBLIC const Loggers slog; +extern UTILS_PUBLIC Loggers const slog; } // namespace utils diff --git a/libs/utils/include/utils/ostream.h b/libs/utils/include/utils/ostream.h index b59dd05eedd..4d1a9b9035d 100644 --- a/libs/utils/include/utils/ostream.h +++ b/libs/utils/include/utils/ostream.h @@ -70,14 +70,14 @@ class UTILS_PUBLIC ostream { Buffer(const Buffer&) = delete; Buffer& operator=(const Buffer&) = delete; - char* buffer; - char* curr; - size_t size = 0; - size_t capacity = 0; + char* buffer = nullptr; // buffer address + char* curr = nullptr; // current pointer + size_t size = 0; // size remaining + size_t capacity = 0; // total capacity of the buffer const char* get() const noexcept { return buffer; } void advance(ssize_t n) noexcept; void reset() noexcept; - void resize(size_t newSize) noexcept; + void reserve(size_t newSize) noexcept; }; Buffer mData; diff --git a/libs/utils/src/Log.cpp b/libs/utils/src/Log.cpp index 81e917283c7..1d100e65578 100644 --- a/libs/utils/src/Log.cpp +++ b/libs/utils/src/Log.cpp @@ -30,6 +30,21 @@ namespace utils { namespace io { +class LogStream : public ostream { +public: + + enum Priority { + LOG_DEBUG, LOG_ERROR, LOG_WARNING, LOG_INFO + }; + + explicit LogStream(Priority p) noexcept : mPriority(p) {} + + ostream& flush() noexcept override; + +private: + Priority mPriority; +}; + ostream& LogStream::flush() noexcept { Buffer& buf = getBuffer(); #if ANDROID @@ -71,7 +86,7 @@ static LogStream cinfo(LogStream::Priority::LOG_INFO); } // namespace io -const Loggers slog = { +Loggers const slog = { io::cout, // debug io::cerr, // error io::cwarn, // warning diff --git a/libs/utils/src/ostream.cpp b/libs/utils/src/ostream.cpp index 102f675f6cf..5eeaf67f2c6 100644 --- a/libs/utils/src/ostream.cpp +++ b/libs/utils/src/ostream.cpp @@ -15,9 +15,10 @@ */ #include +#include +#include #include -#include namespace utils { @@ -25,18 +26,12 @@ namespace io { ostream::~ostream() = default; -ostream::Buffer::Buffer() noexcept { - constexpr size_t initialSize = 1024; - buffer = (char*) malloc(initialSize); - assert(buffer); - // Set the first byte to 0 as this buffer might be used as a C string. - buffer[0] = 0; - curr = buffer; - capacity = initialSize; - size = initialSize; -} +// don't allocate any memory before we actually use the log because one of these is created +// per thread. +ostream::Buffer::Buffer() noexcept = default; ostream::Buffer::~Buffer() noexcept { + // note: on Android pre r14, thread_local destructors are not called free(buffer); } @@ -50,9 +45,13 @@ void ostream::Buffer::advance(ssize_t n) noexcept { } UTILS_NOINLINE -void ostream::Buffer::resize(size_t newSize) noexcept { +void ostream::Buffer::reserve(size_t newSize) noexcept { size_t offset = curr - buffer; - buffer = (char*) realloc(buffer, newSize); + if (buffer == nullptr) { + buffer = (char*)malloc(newSize); + } else { + buffer = (char*)realloc(buffer, newSize); + } assert(buffer); capacity = newSize; curr = buffer + offset; @@ -61,6 +60,12 @@ void ostream::Buffer::resize(size_t newSize) noexcept { UTILS_NOINLINE void ostream::Buffer::reset() noexcept { + // aggressively shrink the buffer + if (capacity > 1024) { + free(buffer); + buffer = (char*)malloc(1024); + capacity = 1024; + } curr = buffer; size = capacity; } @@ -84,13 +89,10 @@ const char* ostream::getFormat(ostream::type t) const noexcept { void ostream::growBufferIfNeeded(size_t s) noexcept { Buffer& buf = getBuffer(); - const size_t used = buf.curr - buf.buffer; // space currently used in buffer if (UTILS_UNLIKELY(buf.size < s)) { - size_t newSize = buf.capacity * 2; - while (UTILS_UNLIKELY((newSize - used) < s)) { - newSize *= 2; - } - buf.resize(newSize); + size_t used = buf.curr - buf.buffer; + size_t newCapacity = std::max(size_t(32), used + (s * 3 + 1) / 2); // 32 bytes minimum + buf.reserve(newCapacity); assert(buf.size >= s); } } From 831a6d94804888d16f1316c2e7ba9a12c0281b81 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Wed, 2 Jun 2021 13:40:18 -0700 Subject: [PATCH 12/24] gltfio: fix crash with non-triangles. Fixes the "triangle count is zero" assertion with the big car model. We do have nominal support for points and lines, but we should not attempt to compute tangents for these. As a reminder, glTF also supports loops, strips, and fans; this is the only way in which Filament is non-conformant. --- libs/gltfio/src/ResourceLoader.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/gltfio/src/ResourceLoader.cpp b/libs/gltfio/src/ResourceLoader.cpp index b27ccf316b7..0ea852515c9 100644 --- a/libs/gltfio/src/ResourceLoader.cpp +++ b/libs/gltfio/src/ResourceLoader.cpp @@ -873,10 +873,13 @@ void ResourceLoader::Impl::computeTangents(FFilamentAsset* asset) { baseTangents[slot.vertexBuffer] = slot.bufferIndex; } - // Create a job description for each primitive. + // Create a job description for each triangle-based primitive. using Params = TangentsJob::Params; std::vector jobParams; for (auto pair : asset->mPrimitives) { + if (UTILS_UNLIKELY(pair.first->type != cgltf_primitive_type_triangles)) { + continue; + } VertexBuffer* vb = pair.second; auto iter = baseTangents.find(vb); if (iter != baseTangents.end()) { From 29c036d826d5630afdf26198868602772f00e393 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Tue, 1 Jun 2021 17:28:06 -0700 Subject: [PATCH 13/24] VulkanPipelineCache: do not hash uninitialized data. This fixes an issue caught by a memory sanitizer, whereby padding in the cache key structure was being consumed by a hashing function. In practice this oversight was fairly harmless, but this change makes the code more safe and predictable. --- filament/backend/src/vulkan/VulkanPipelineCache.cpp | 1 + filament/backend/src/vulkan/VulkanPipelineCache.h | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.cpp b/filament/backend/src/vulkan/VulkanPipelineCache.cpp index 3c234fe993d..a3fa100ed6d 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.cpp +++ b/filament/backend/src/vulkan/VulkanPipelineCache.cpp @@ -39,6 +39,7 @@ VulkanPipelineCache::VulkanPipelineCache() : mDefaultRasterState(createDefaultRa markDirtyDescriptor(); markDirtyPipeline(); mDescriptorKey = {}; + mPipelineKey = {}; mDummyBufferWriteInfo.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET; mDummyBufferWriteInfo.pNext = nullptr; diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.h b/filament/backend/src/vulkan/VulkanPipelineCache.h index d7fee8de931..4c3bcd5a427 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.h +++ b/filament/backend/src/vulkan/VulkanPipelineCache.h @@ -77,13 +77,15 @@ class VulkanPipelineCache : public CommandBufferObserver { // The RasterState POD contains standard graphics-related state like blending, culling, etc. // Note that several fields are unused (sType etc) so we could shrink this by avoiding the Vk // structures. However it's super convenient just to use standard Vulkan structs here. - struct alignas(8) RasterState { + #pragma pack(push, 1) + struct UTILS_PACKED RasterState { VkPipelineRasterizationStateCreateInfo rasterization; VkPipelineColorBlendAttachmentState blending; VkPipelineDepthStencilStateCreateInfo depthStencil; VkPipelineMultisampleStateCreateInfo multisampling; uint32_t colorTargetCount; }; + #pragma pack(pop) static_assert(std::is_pod::value, "RasterState must be a POD for fast hashing."); // Upon construction, the pipeCache initializes some internal state but does not make any Vulkan From 28e414c9a512961c5de53599dc7d2d1795da3eec Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Wed, 2 Jun 2021 10:15:44 -0700 Subject: [PATCH 14/24] VulkanPipelineCache: stop using Vk structs for state tracking. --- filament/backend/src/vulkan/VulkanDriver.cpp | 4 +- .../src/vulkan/VulkanPipelineCache.cpp | 196 +++++++++++------- .../backend/src/vulkan/VulkanPipelineCache.h | 67 ++++-- 3 files changed, 173 insertions(+), 94 deletions(-) diff --git a/filament/backend/src/vulkan/VulkanDriver.cpp b/filament/backend/src/vulkan/VulkanDriver.cpp index 9e0f21f18cf..3a39be837d8 100644 --- a/filament/backend/src/vulkan/VulkanDriver.cpp +++ b/filament/backend/src/vulkan/VulkanDriver.cpp @@ -1645,7 +1645,6 @@ void VulkanDriver::draw(PipelineState pipelineState, Handle r const VulkanRenderTarget* rt = mCurrentRenderTarget; mContext.rasterState.depthStencil = { - .sType = VK_STRUCTURE_TYPE_PIPELINE_DEPTH_STENCIL_STATE_CREATE_INFO, .depthTestEnable = VK_TRUE, .depthWriteEnable = (VkBool32) rasterState.depthWrite, .depthCompareOp = getCompareOp(rasterState.depthFunc), @@ -1654,7 +1653,6 @@ void VulkanDriver::draw(PipelineState pipelineState, Handle r }; mContext.rasterState.multisampling = { - .sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO, .rasterizationSamples = (VkSampleCountFlagBits) rt->getSamples(), .alphaToCoverageEnable = rasterState.alphaToCoverage, }; @@ -1670,7 +1668,7 @@ void VulkanDriver::draw(PipelineState pipelineState, Handle r .colorWriteMask = (VkColorComponentFlags) (rasterState.colorWrite ? 0xf : 0x0), }; - VkPipelineRasterizationStateCreateInfo& vkraster = mContext.rasterState.rasterization; + auto& vkraster = mContext.rasterState.rasterization; vkraster.cullMode = getCullMode(rasterState.culling); vkraster.frontFace = getFrontFace(rasterState.inverseFrontFaces); vkraster.depthBiasEnable = (depthOffset.constant || depthOffset.slope) ? VK_TRUE : VK_FALSE; diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.cpp b/filament/backend/src/vulkan/VulkanPipelineCache.cpp index a3fa100ed6d..8fd71096b94 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.cpp +++ b/filament/backend/src/vulkan/VulkanPipelineCache.cpp @@ -83,8 +83,9 @@ bool VulkanPipelineCache::bindDescriptors(VkCommandBuffer cmdbuffer) noexcept { return false; } if (bind) { - vkCmdBindDescriptorSets(cmdbuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, mPipelineLayout, 0, - VulkanPipelineCache::DESCRIPTOR_TYPE_COUNT, descriptors, 0, nullptr); + vkCmdBindDescriptorSets(cmdbuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, + mPipelineLayout, 0, VulkanPipelineCache::DESCRIPTOR_TYPE_COUNT, descriptors, + 0, nullptr); } return true; } @@ -374,11 +375,57 @@ bool VulkanPipelineCache::getOrCreatePipeline(VkPipeline* pipeline) noexcept { pipelineCreateInfo.pStages = shaderStages; pipelineCreateInfo.pVertexInputState = &vertexInputState; pipelineCreateInfo.pInputAssemblyState = &inputAssemblyState; - pipelineCreateInfo.pRasterizationState = &mPipelineKey.rasterState.rasterization; + + VkPipelineRasterizationStateCreateInfo vkRaster = {}; + vkRaster.sType = VK_STRUCTURE_TYPE_PIPELINE_RASTERIZATION_STATE_CREATE_INFO; + pipelineCreateInfo.pRasterizationState = &vkRaster; + + VkPipelineMultisampleStateCreateInfo vkMs = {}; + vkMs.sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO; + pipelineCreateInfo.pMultisampleState = &vkMs; + + VkPipelineDepthStencilStateCreateInfo vkDs = {}; + vkDs.sType = VK_STRUCTURE_TYPE_PIPELINE_DEPTH_STENCIL_STATE_CREATE_INFO; + vkDs.front = vkDs.back = { + .failOp = VK_STENCIL_OP_KEEP, + .passOp = VK_STENCIL_OP_KEEP, + .depthFailOp = VK_STENCIL_OP_KEEP, + .compareOp = VK_COMPARE_OP_ALWAYS, + .compareMask = 0u, + .writeMask = 0u, + .reference = 0u, + }; + pipelineCreateInfo.pDepthStencilState = &vkDs; + + const auto& raster = mPipelineKey.rasterState; + + vkRaster.depthClampEnable = raster.rasterization.depthClampEnable; + vkRaster.rasterizerDiscardEnable = raster.rasterization.rasterizerDiscardEnable; + vkRaster.polygonMode = raster.rasterization.polygonMode; + vkRaster.cullMode = raster.rasterization.cullMode; + vkRaster.frontFace = raster.rasterization.frontFace; + vkRaster.depthBiasEnable = raster.rasterization.depthBiasEnable; + vkRaster.depthBiasConstantFactor = raster.rasterization.depthBiasConstantFactor; + vkRaster.depthBiasClamp = raster.rasterization.depthBiasClamp; + vkRaster.depthBiasSlopeFactor = raster.rasterization.depthBiasSlopeFactor; + vkRaster.lineWidth = raster.rasterization.lineWidth; + + vkMs.rasterizationSamples = raster.multisampling.rasterizationSamples; + vkMs.sampleShadingEnable = raster.multisampling.sampleShadingEnable; + vkMs.minSampleShading = raster.multisampling.minSampleShading; + vkMs.alphaToCoverageEnable = raster.multisampling.alphaToCoverageEnable; + vkMs.alphaToOneEnable = raster.multisampling.alphaToOneEnable; + + vkDs.depthTestEnable = raster.depthStencil.depthTestEnable; + vkDs.depthWriteEnable = raster.depthStencil.depthWriteEnable; + vkDs.depthCompareOp = raster.depthStencil.depthCompareOp; + vkDs.depthBoundsTestEnable = raster.depthStencil.depthBoundsTestEnable; + vkDs.stencilTestEnable = raster.depthStencil.stencilTestEnable; + vkDs.minDepthBounds = raster.depthStencil.minDepthBounds; + vkDs.maxDepthBounds = raster.depthStencil.maxDepthBounds; + pipelineCreateInfo.pColorBlendState = &colorBlendState; - pipelineCreateInfo.pMultisampleState = &mPipelineKey.rasterState.multisampling; pipelineCreateInfo.pViewportState = &viewportState; - pipelineCreateInfo.pDepthStencilState = &mPipelineKey.rasterState.depthStencil; pipelineCreateInfo.pDynamicState = &dynamicState; // Filament assumes consistent blend state across all color attachments. @@ -424,32 +471,51 @@ void VulkanPipelineCache::bindProgramBundle(const ProgramBundle& bundle) noexcep } void VulkanPipelineCache::bindRasterState(const RasterState& rasterState) noexcept { - VkPipelineRasterizationStateCreateInfo& raster0 = mPipelineKey.rasterState.rasterization; - const VkPipelineRasterizationStateCreateInfo& raster1 = rasterState.rasterization; - VkPipelineColorBlendAttachmentState& blend0 = mPipelineKey.rasterState.blending; - const VkPipelineColorBlendAttachmentState& blend1 = rasterState.blending; - VkPipelineDepthStencilStateCreateInfo& ds0 = mPipelineKey.rasterState.depthStencil; - const VkPipelineDepthStencilStateCreateInfo& ds1 = rasterState.depthStencil; - VkPipelineMultisampleStateCreateInfo& ms0 = mPipelineKey.rasterState.multisampling; - const VkPipelineMultisampleStateCreateInfo& ms1 = rasterState.multisampling; - if ( - mPipelineKey.rasterState.colorTargetCount != rasterState.colorTargetCount || - raster0.polygonMode != raster1.polygonMode || - raster0.cullMode != raster1.cullMode || - raster0.frontFace != raster1.frontFace || - raster0.rasterizerDiscardEnable != raster1.rasterizerDiscardEnable || - raster0.depthBiasEnable != raster1.depthBiasEnable || - raster0.depthBiasConstantFactor != raster1.depthBiasConstantFactor || - raster0.depthBiasSlopeFactor != raster1.depthBiasSlopeFactor || - blend0.colorWriteMask != blend1.colorWriteMask || - blend0.blendEnable != blend1.blendEnable || - ds0.depthTestEnable != ds1.depthTestEnable || - ds0.depthWriteEnable != ds1.depthWriteEnable || - ds0.depthCompareOp != ds1.depthCompareOp || - ds0.stencilTestEnable != ds1.stencilTestEnable || - ms0.rasterizationSamples != ms1.rasterizationSamples || - ms0.alphaToCoverageEnable != ms1.alphaToCoverageEnable - ) { + auto& raster0 = mPipelineKey.rasterState.rasterization; + const auto& raster1 = rasterState.rasterization; + auto& blend0 = mPipelineKey.rasterState.blending; + const auto& blend1 = rasterState.blending; + auto& ds0 = mPipelineKey.rasterState.depthStencil; + const auto& ds1 = rasterState.depthStencil; + auto& ms0 = mPipelineKey.rasterState.multisampling; + const auto& ms1 = rasterState.multisampling; + if (UTILS_UNLIKELY( + raster0.depthClampEnable != raster1.depthClampEnable || + raster0.rasterizerDiscardEnable != raster1.rasterizerDiscardEnable || + raster0.polygonMode != raster1.polygonMode || + raster0.cullMode != raster1.cullMode || + raster0.frontFace != raster1.frontFace || + raster0.depthBiasEnable != raster1.depthBiasEnable || + raster0.depthBiasConstantFactor != raster1.depthBiasConstantFactor || + raster0.depthBiasClamp != raster1.depthBiasClamp || + raster0.depthBiasSlopeFactor != raster1.depthBiasSlopeFactor || + raster0.lineWidth != raster1.lineWidth || + + blend0.blendEnable != blend1.blendEnable || + blend0.srcColorBlendFactor != blend1.srcColorBlendFactor || + blend0.dstColorBlendFactor != blend1.dstColorBlendFactor || + blend0.colorBlendOp != blend1.colorBlendOp || + blend0.srcAlphaBlendFactor != blend1.srcAlphaBlendFactor || + blend0.dstAlphaBlendFactor != blend1.dstAlphaBlendFactor || + blend0.alphaBlendOp != blend1.alphaBlendOp || + blend0.colorWriteMask != blend1.colorWriteMask || + + ds0.depthTestEnable != ds1.depthTestEnable || + ds0.depthWriteEnable != ds1.depthWriteEnable || + ds0.depthCompareOp != ds1.depthCompareOp || + ds0.depthBoundsTestEnable != ds1.depthBoundsTestEnable || + ds0.stencilTestEnable != ds1.stencilTestEnable || + ds0.minDepthBounds != ds1.minDepthBounds || + ds0.maxDepthBounds != ds1.maxDepthBounds || + + ms0.rasterizationSamples != ms1.rasterizationSamples || + ms0.sampleShadingEnable != ms1.sampleShadingEnable || + ms0.minSampleShading != ms1.minSampleShading || + ms0.alphaToCoverageEnable != ms1.alphaToCoverageEnable || + ms0.alphaToOneEnable != ms1.alphaToOneEnable || + + mPipelineKey.rasterState.colorTargetCount != rasterState.colorTargetCount + )) { markDirtyPipeline(); mPipelineKey.rasterState = rasterState; } @@ -826,47 +892,35 @@ bool VulkanPipelineCache::DescEqual::operator()(const VulkanPipelineCache::Descr } static VulkanPipelineCache::RasterState createDefaultRasterState() { - VkPipelineRasterizationStateCreateInfo rasterization = {}; - rasterization.sType = VK_STRUCTURE_TYPE_PIPELINE_RASTERIZATION_STATE_CREATE_INFO; - rasterization.polygonMode = VK_POLYGON_MODE_FILL; - rasterization.cullMode = VK_CULL_MODE_NONE; - rasterization.frontFace = VK_FRONT_FACE_COUNTER_CLOCKWISE; - rasterization.depthClampEnable = VK_FALSE; - rasterization.rasterizerDiscardEnable = VK_FALSE; - rasterization.depthBiasEnable = VK_FALSE; - rasterization.depthBiasConstantFactor = 0.0f; - rasterization.depthBiasClamp = 0.0f; // 0 is a special value that disables clamping - rasterization.depthBiasSlopeFactor = 0.0f; - rasterization.lineWidth = 1.0f; - - VkPipelineColorBlendAttachmentState blending = {}; - blending.colorWriteMask = 0xf; - blending.blendEnable = VK_FALSE; - - VkPipelineDepthStencilStateCreateInfo depthStencil = {}; - depthStencil.sType = VK_STRUCTURE_TYPE_PIPELINE_DEPTH_STENCIL_STATE_CREATE_INFO; - depthStencil.depthTestEnable = VK_TRUE; - depthStencil.depthWriteEnable = VK_TRUE; - depthStencil.depthCompareOp = VK_COMPARE_OP_LESS_OR_EQUAL; - depthStencil.depthBoundsTestEnable = VK_FALSE; - depthStencil.back.failOp = VK_STENCIL_OP_KEEP; - depthStencil.back.passOp = VK_STENCIL_OP_KEEP; - depthStencil.back.compareOp = VK_COMPARE_OP_ALWAYS; - depthStencil.stencilTestEnable = VK_FALSE; - depthStencil.front = depthStencil.back; - - VkPipelineMultisampleStateCreateInfo multisampling = {}; - multisampling.sType = VK_STRUCTURE_TYPE_PIPELINE_MULTISAMPLE_STATE_CREATE_INFO; - multisampling.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT; - multisampling.pSampleMask = nullptr; - multisampling.alphaToCoverageEnable = true; - return VulkanPipelineCache::RasterState { - rasterization, - blending, - depthStencil, - multisampling, - 1, + .rasterization = { + .depthClampEnable = VK_FALSE, + .rasterizerDiscardEnable = VK_FALSE, + .polygonMode = VK_POLYGON_MODE_FILL, + .cullMode = VK_CULL_MODE_NONE, + .frontFace = VK_FRONT_FACE_COUNTER_CLOCKWISE, + .depthBiasEnable = VK_FALSE, + .depthBiasConstantFactor = 0.0f, + .depthBiasClamp = 0.0f, + .depthBiasSlopeFactor = 0.0f, + .lineWidth = 1.0f, + }, + .blending = { + .blendEnable = VK_FALSE, + .colorWriteMask = 0xf, + }, + .depthStencil = { + .depthTestEnable = VK_TRUE, + .depthWriteEnable = VK_TRUE, + .depthCompareOp = VK_COMPARE_OP_LESS_OR_EQUAL, + .depthBoundsTestEnable = VK_FALSE, + .stencilTestEnable = VK_FALSE, + }, + .multisampling = { + .rasterizationSamples = VK_SAMPLE_COUNT_1_BIT, + .alphaToCoverageEnable = true, + }, + .colorTargetCount = 1, }; } diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.h b/filament/backend/src/vulkan/VulkanPipelineCache.h index 4c3bcd5a427..4f06f60ab06 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.h +++ b/filament/backend/src/vulkan/VulkanPipelineCache.h @@ -29,6 +29,7 @@ #include #include +#include #include #include "VulkanCommands.h" @@ -75,18 +76,43 @@ class VulkanPipelineCache : public CommandBufferObserver { }; // The RasterState POD contains standard graphics-related state like blending, culling, etc. - // Note that several fields are unused (sType etc) so we could shrink this by avoiding the Vk - // structures. However it's super convenient just to use standard Vulkan structs here. - #pragma pack(push, 1) - struct UTILS_PACKED RasterState { - VkPipelineRasterizationStateCreateInfo rasterization; - VkPipelineColorBlendAttachmentState blending; - VkPipelineDepthStencilStateCreateInfo depthStencil; - VkPipelineMultisampleStateCreateInfo multisampling; + struct RasterState { + struct { + VkBool32 depthClampEnable; + VkBool32 rasterizerDiscardEnable; + VkPolygonMode polygonMode; + VkCullModeFlags cullMode; + VkFrontFace frontFace; + VkBool32 depthBiasEnable; + float depthBiasConstantFactor; + float depthBiasClamp; + float depthBiasSlopeFactor; + float lineWidth; + } rasterization; // 40 bytes + VkPipelineColorBlendAttachmentState blending; // 32 bytes + struct { + VkBool32 depthTestEnable; + VkBool32 depthWriteEnable; + VkCompareOp depthCompareOp; + VkBool32 depthBoundsTestEnable; + VkBool32 stencilTestEnable; + float minDepthBounds; + float maxDepthBounds; + } depthStencil; // 28 bytes + struct { + VkSampleCountFlagBits rasterizationSamples; + VkBool32 sampleShadingEnable; + float minSampleShading; + VkBool32 alphaToCoverageEnable; + VkBool32 alphaToOneEnable; + } multisampling; // 20 bytes uint32_t colorTargetCount; }; - #pragma pack(pop) - static_assert(std::is_pod::value, "RasterState must be a POD for fast hashing."); + + static_assert(std::is_trivially_copyable::value, + "RasterState must be a POD for fast hashing."); + + static_assert(sizeof(RasterState) == 124, "RasterState must not have any padding."); // Upon construction, the pipeCache initializes some internal state but does not make any Vulkan // calls. On destruction it will free any cached Vulkan objects that haven't already been freed. @@ -148,19 +174,20 @@ class VulkanPipelineCache : public CommandBufferObserver { // The pipeline key is a POD that represents all currently bound states that form the immutable // VkPipeline object. We apply a hash function to its contents only if has been mutated since // the previous call to getOrCreatePipeline. - #pragma pack(push, 1) - struct UTILS_PACKED PipelineKey { - VkShaderModule shaders[SHADER_MODULE_COUNT]; // 8*2 bytes - RasterState rasterState; // 248 bytes - VkRenderPass renderPass; // 8 bytes + struct PipelineKey { + VkShaderModule shaders[SHADER_MODULE_COUNT]; // 16 bytes + RasterState rasterState; // 124 bytes VkPrimitiveTopology topology : 16; // 2 bytes uint16_t subpassIndex; // 2 bytes - VkVertexInputAttributeDescription vertexAttributes[VERTEX_ATTRIBUTE_COUNT]; // 16*5 bytes - VkVertexInputBindingDescription vertexBuffers[VERTEX_ATTRIBUTE_COUNT]; // 12*5 bytes + VkRenderPass renderPass; // 8 bytes + VkVertexInputAttributeDescription vertexAttributes[VERTEX_ATTRIBUTE_COUNT]; // 256 bytes + VkVertexInputBindingDescription vertexBuffers[VERTEX_ATTRIBUTE_COUNT]; // 192 bytes }; - #pragma pack(pop) - static_assert(std::is_pod::value, "PipelineKey must be a POD for fast hashing."); + static_assert(sizeof(PipelineKey) == 600, "PipelineKey must not have any padding."); + + static_assert(std::is_trivially_copyable::value, + "PipelineKey must be a POD for fast hashing."); using PipelineHashFn = utils::hash::MurmurHashFn; @@ -181,7 +208,7 @@ class VulkanPipelineCache : public CommandBufferObserver { }; #pragma pack(pop) - static_assert(std::is_pod::value, "DescriptorKey must be a POD."); + static_assert(std::is_trivially_copyable::value, "DescriptorKey must be a POD."); using DescHashFn = utils::hash::MurmurHashFn; From 46029acd140df3647717a1824e289f2305383ca6 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Wed, 2 Jun 2021 15:23:33 -0700 Subject: [PATCH 15/24] VulkanPipelineCache: fix padding with MSVC. --- .../backend/src/vulkan/VulkanPipelineCache.h | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/filament/backend/src/vulkan/VulkanPipelineCache.h b/filament/backend/src/vulkan/VulkanPipelineCache.h index 4f06f60ab06..bc055c1fb4e 100644 --- a/filament/backend/src/vulkan/VulkanPipelineCache.h +++ b/filament/backend/src/vulkan/VulkanPipelineCache.h @@ -176,15 +176,25 @@ class VulkanPipelineCache : public CommandBufferObserver { // the previous call to getOrCreatePipeline. struct PipelineKey { VkShaderModule shaders[SHADER_MODULE_COUNT]; // 16 bytes - RasterState rasterState; // 124 bytes - VkPrimitiveTopology topology : 16; // 2 bytes - uint16_t subpassIndex; // 2 bytes - VkRenderPass renderPass; // 8 bytes + RasterState rasterState; // 124 bytes + VkPrimitiveTopology topology; // 4 bytes + VkRenderPass renderPass; // 8 bytes + uint16_t subpassIndex; // 2 bytes + uint16_t padding0; // 2 bytes VkVertexInputAttributeDescription vertexAttributes[VERTEX_ATTRIBUTE_COUNT]; // 256 bytes - VkVertexInputBindingDescription vertexBuffers[VERTEX_ATTRIBUTE_COUNT]; // 192 bytes + VkVertexInputBindingDescription vertexBuffers[VERTEX_ATTRIBUTE_COUNT]; // 192 bytes + uint32_t padding1; // 4 bytes }; - static_assert(sizeof(PipelineKey) == 600, "PipelineKey must not have any padding."); + static_assert(sizeof(VkVertexInputBindingDescription) == 12); + + static_assert(offsetof(PipelineKey, rasterState) == 16); + static_assert(offsetof(PipelineKey, topology) == 140); + static_assert(offsetof(PipelineKey, renderPass) == 144); + static_assert(offsetof(PipelineKey, subpassIndex) == 152); + static_assert(offsetof(PipelineKey, vertexAttributes) == 156); + static_assert(offsetof(PipelineKey, vertexBuffers) == 412); + static_assert(sizeof(PipelineKey) == 608, "PipelineKey must not have any padding."); static_assert(std::is_trivially_copyable::value, "PipelineKey must be a POD for fast hashing."); From 5056abc357e3a458b9fcaca28b364e282f41c22e Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Wed, 2 Jun 2021 16:38:12 -0700 Subject: [PATCH 16/24] Systrace now uses the official ATrace API Use the official systrace API when available. --- libs/utils/CMakeLists.txt | 1 + libs/utils/include/utils/Systrace.h | 89 +++++++++----- libs/utils/src/Systrace.cpp | 176 ++++++++++++++++++++-------- 3 files changed, 191 insertions(+), 75 deletions(-) diff --git a/libs/utils/CMakeLists.txt b/libs/utils/CMakeLists.txt index bc3e9a94909..6869bcb1347 100644 --- a/libs/utils/CMakeLists.txt +++ b/libs/utils/CMakeLists.txt @@ -90,6 +90,7 @@ target_link_libraries(${TARGET} PUBLIC tsl) if (ANDROID) target_link_libraries(${TARGET} PUBLIC log) target_link_libraries(${TARGET} PRIVATE dl) + target_link_libraries(${TARGET} PUBLIC android) endif() if (WIN32) diff --git a/libs/utils/include/utils/Systrace.h b/libs/utils/include/utils/Systrace.h index d89e309283a..2271f2790b4 100644 --- a/libs/utils/include/utils/Systrace.h +++ b/libs/utils/include/utils/Systrace.h @@ -125,74 +125,107 @@ class Systrace { static void enable(uint32_t tags) noexcept; static void disable(uint32_t tags) noexcept; - inline void asyncBegin(uint32_t tag, const char* name, int32_t cookie) noexcept { + + inline void traceBegin(uint32_t tag, const char* name) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - async_begin_body(mMarkerFd, mPid, name, cookie); + beginSection(this, name); } } - inline void asyncEnd(uint32_t tag, const char* name, int32_t cookie) noexcept { + inline void traceEnd(uint32_t tag) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - async_end_body(mMarkerFd, mPid, name, cookie); + endSection(this); } } - inline void value(uint32_t tag, const char* name, int32_t value) noexcept { + inline void asyncBegin(uint32_t tag, const char* name, int32_t cookie) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - int_body(mMarkerFd, mPid, name, value); + beginAsyncSection(this, name, cookie); } } - inline void value(uint32_t tag, const char* name, int64_t value) noexcept { + inline void asyncEnd(uint32_t tag, const char* name, int32_t cookie) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - int64_body(mMarkerFd, mPid, name, value); + endAsyncSection(this, name, cookie); } } - inline void traceBegin(uint32_t tag, const char* name) noexcept { + inline void value(uint32_t tag, const char* name, int32_t value) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - begin_body(mMarkerFd, mPid, name); + setCounter(this, name, value); } } - inline void traceEnd(uint32_t tag) noexcept { + inline void value(uint32_t tag, const char* name, int64_t value) noexcept { if (tag && UTILS_UNLIKELY(mIsTracingEnabled)) { - const char END_TAG = 'E'; - write(mMarkerFd, &END_TAG, 1); + setCounter(this, name, value); } } private: friend class ScopedTrace; - static inline void init() noexcept { - if (UTILS_UNLIKELY(!std::atomic_load_explicit(&sIsTracingReady, std::memory_order_acquire))) { - setup(); - } - } + // whether tracing is supported at all by the platform + + using ATrace_isEnabled_t = bool (*)(void); + using ATrace_beginSection_t = void (*)(const char* sectionName); + using ATrace_endSection_t = void (*)(void); + using ATrace_beginAsyncSection_t = void (*)(const char* sectionName, int32_t cookie); + using ATrace_endAsyncSection_t = void (*)(const char* sectionName, int32_t cookie); + using ATrace_setCounter_t = void (*)(const char* counterName, int64_t counterValue); + + struct GlobalState { + bool isTracingAvailable; + std::atomic isTracingEnabled; + int markerFd; + + ATrace_isEnabled_t ATrace_isEnabled; + ATrace_beginSection_t ATrace_beginSection; + ATrace_endSection_t ATrace_endSection; + ATrace_beginAsyncSection_t ATrace_beginAsyncSection; + ATrace_endAsyncSection_t ATrace_endAsyncSection; + ATrace_setCounter_t ATrace_setCounter; + + void (*beginSection)(Systrace* that, const char* name); + void (*endSection)(Systrace* that); + void (*beginAsyncSection)(Systrace* that, const char* name, int32_t cookie); + void (*endAsyncSection)(Systrace* that, const char* name, int32_t cookie); + void (*setCounter)(Systrace* that, const char* name, int64_t value); + }; + + static GlobalState sGlobalState; - void init(uint32_t tag) noexcept; - static std::atomic_bool sIsTracingReady; - static int sMarkerFd; - static bool sIsTracingAvailable; - static std::atomic sIsTracingEnabled; + // per-instance versions for better performance + ATrace_isEnabled_t ATrace_isEnabled; + ATrace_beginSection_t ATrace_beginSection; + ATrace_endSection_t ATrace_endSection; + ATrace_beginAsyncSection_t ATrace_beginAsyncSection; + ATrace_endAsyncSection_t ATrace_endAsyncSection; + ATrace_setCounter_t ATrace_setCounter; + + void (*beginSection)(Systrace* that, const char* name); + void (*endSection)(Systrace* that); + void (*beginAsyncSection)(Systrace* that, const char* name, int32_t cookie); + void (*endAsyncSection)(Systrace* that, const char* name, int32_t cookie); + void (*setCounter)(Systrace* that, const char* name, int64_t value); + + void init(uint32_t tag) noexcept; // cached values for faster access, no need to be initialized - int mMarkerFd; - int mPid; bool mIsTracingEnabled; + int mMarkerFd = -1; + pid_t mPid; static void setup() noexcept; static void init_once() noexcept; + static bool isTracingEnabled(uint32_t tag) noexcept; static void begin_body(int fd, int pid, const char* name) noexcept; + static void end_body(int fd, int pid) noexcept; static void async_begin_body(int fd, int pid, const char* name, int32_t cookie) noexcept; static void async_end_body(int fd, int pid, const char* name, int32_t cookie) noexcept; - static void int_body(int fd, int pid, const char* name, int32_t value) noexcept; static void int64_body(int fd, int pid, const char* name, int64_t value) noexcept; - - static bool isTracingEnabled(uint32_t tag) noexcept; }; // ------------------------------------------------------------------------------------------------ diff --git a/libs/utils/src/Systrace.cpp b/libs/utils/src/Systrace.cpp index faeb4502147..58cfa149660 100644 --- a/libs/utils/src/Systrace.cpp +++ b/libs/utils/src/Systrace.cpp @@ -26,32 +26,94 @@ #include #include #include +#include namespace utils { namespace details { -/** - * Maximum size of a message that can be logged to the trace buffer. - * Note this message includes a tag, the pid, and the string given as the name. - * Names should be kept short to get the most use of the trace buffer. - */ -#define ATRACE_MESSAGE_LENGTH 512 +static pthread_once_t atrace_once_control = PTHREAD_ONCE_INIT; -std::atomic_bool Systrace::sIsTracingReady = { false }; -std::atomic Systrace::sIsTracingEnabled = { 0 }; -bool Systrace::sIsTracingAvailable = false; -int Systrace::sMarkerFd = -1; +template +static void loadSymbol(T*& pfn, const char *symbol) noexcept { + pfn = (T*)dlsym(RTLD_DEFAULT, symbol); +} -static pthread_once_t atrace_once_control = PTHREAD_ONCE_INIT; +Systrace::GlobalState Systrace::sGlobalState = {}; void Systrace::init_once() noexcept { - sMarkerFd = open("/sys/kernel/debug/tracing/trace_marker", O_WRONLY | O_CLOEXEC); - if (UTILS_UNLIKELY(sMarkerFd == -1)) { - slog.e << "Error opening trace file: " << strerror(errno) << " (" << errno << ")" << io::endl; - } else { - sIsTracingAvailable = true; + GlobalState& s = sGlobalState; + + s.markerFd = -1; + + // API 23 + loadSymbol(s.ATrace_isEnabled, "ATrace_isEnabled"); + loadSymbol(s.ATrace_beginSection, "ATrace_beginSection"); + loadSymbol(s.ATrace_endSection, "ATrace_endSection"); + // API 29 + loadSymbol(s.ATrace_beginAsyncSection, "ATrace_beginAsyncSection"); + loadSymbol(s.ATrace_endAsyncSection, "ATrace_endAsyncSection"); + loadSymbol(s.ATrace_setCounter, "ATrace_setCounter"); + + + const bool hasBasicAtrace = s.ATrace_isEnabled && + s.ATrace_beginSection && + s.ATrace_endSection; + + const bool hasFullATrace = hasBasicAtrace && + s.ATrace_beginAsyncSection && + s.ATrace_endAsyncSection && + s.ATrace_setCounter; + + if (!hasFullATrace) { + s.markerFd = open("/sys/kernel/debug/tracing/trace_marker", O_WRONLY | O_CLOEXEC); + } + + if (hasBasicAtrace && !hasFullATrace) { + // no-op if we don't have all these + s.ATrace_beginAsyncSection = [](const char* sectionName, int32_t cookie){}; + s.ATrace_endAsyncSection = [](const char* sectionName, int32_t cookie){}; + s.ATrace_setCounter = [](const char* sectionName, int64_t counterValue){}; } - std::atomic_store_explicit(&sIsTracingReady, true, std::memory_order_release); + + const bool hasLegacySystrace = s.markerFd != -1; + + if (hasLegacySystrace && !hasFullATrace) { + // use legacy + s.beginSection = [](Systrace* that, const char* name) { + begin_body(that->mMarkerFd, that->mPid, name); + }; + s.endSection = [](Systrace* that) { + end_body(that->mMarkerFd, that->mPid); + }; + s.beginAsyncSection = [](Systrace* that, const char* name, int32_t cookie) { + async_begin_body(that->mMarkerFd, that->mPid, name, cookie); + }; + s.endAsyncSection = [](Systrace* that, const char* name, int32_t cookie) { + async_end_body(that->mMarkerFd, that->mPid, name, cookie); + }; + s.setCounter = [](Systrace* that, const char* name, int64_t value) { + int64_body(that->mMarkerFd, that->mPid, name, value); + }; + } else if (hasBasicAtrace) { + // we have at least basic ATrace + s.beginSection = [](Systrace* that, const char* name) { + that->ATrace_beginSection(name); + }; + s.endSection = [](Systrace* that) { + that->ATrace_endSection(); + }; + s.beginAsyncSection = [](Systrace* that, const char* name, int32_t cookie) { + that->ATrace_beginAsyncSection(name, cookie); + }; + s.endAsyncSection = [](Systrace* that, const char* name, int32_t cookie) { + that->ATrace_endAsyncSection(name, cookie); + }; + s.setCounter = [](Systrace* that, const char* name, int64_t value) { + that->ATrace_setCounter(name, value); + }; + } + + s.isTracingAvailable = hasLegacySystrace || hasFullATrace || hasBasicAtrace; } void Systrace::setup() noexcept { @@ -59,38 +121,61 @@ void Systrace::setup() noexcept { } void Systrace::enable(uint32_t tags) noexcept { - init(); - if (UTILS_LIKELY(sIsTracingAvailable)) { - sIsTracingEnabled.fetch_or(tags, std::memory_order_relaxed); + setup(); + if (UTILS_LIKELY(sGlobalState.isTracingAvailable)) { + sGlobalState.isTracingEnabled.fetch_or(tags, std::memory_order_relaxed); } } void Systrace::disable(uint32_t tags) noexcept { - sIsTracingEnabled.fetch_and(~tags, std::memory_order_relaxed); + sGlobalState.isTracingEnabled.fetch_and(~tags, std::memory_order_relaxed); } // unfortunately, this generates quite a bit of code because reading a global is not // trivial. For this reason, we do not inline this method. bool Systrace::isTracingEnabled(uint32_t tag) noexcept { if (tag) { - init(); - return bool((sIsTracingEnabled.load(std::memory_order_relaxed) | SYSTRACE_TAG_ALWAYS) & tag); + setup(); + return bool((sGlobalState.isTracingEnabled.load(std::memory_order_relaxed) | SYSTRACE_TAG_ALWAYS) & tag); } return false; } // ------------------------------------------------------------------------------------------------ -void Systrace::begin_body(int fd, int pid, const char* name) noexcept { - char buf[ATRACE_MESSAGE_LENGTH]; +void Systrace::init(uint32_t tag) noexcept { + // must be called first + mIsTracingEnabled = isTracingEnabled(tag); - ssize_t len = snprintf(buf, sizeof(buf), "B|%d|%s", pid, name); - if (len >= sizeof(buf)) { - len = sizeof(buf) - 1; - } - write(fd, buf, size_t(len)); + // cache static variables for better efficiency + GlobalState& s = sGlobalState; + ATrace_isEnabled = s.ATrace_isEnabled; + ATrace_beginSection = s.ATrace_beginSection; + ATrace_endSection = s.ATrace_endSection; + ATrace_beginAsyncSection = s.ATrace_beginAsyncSection; + ATrace_endAsyncSection = s.ATrace_endAsyncSection; + ATrace_setCounter = s.ATrace_setCounter; + + beginSection = s.beginSection; + endSection = s.endSection; + beginAsyncSection = s.beginAsyncSection; + endAsyncSection = s.endAsyncSection; + setCounter = s.setCounter; + + mMarkerFd = s.markerFd; + + mPid = getpid(); } +// ------------------------------------------------------------------------------------------------ + +/** + * Maximum size of a message that can be logged to the trace buffer. + * Note this message includes a tag, the pid, and the string given as the name. + * Names should be kept short to get the most use of the trace buffer. + */ +#define ATRACE_MESSAGE_LENGTH 512 + #define WRITE_MSG(format_begin, format_end, pid, name, value) { \ char buf[ATRACE_MESSAGE_LENGTH]; \ int len = snprintf(buf, sizeof(buf), format_begin "%s" format_end, pid, \ @@ -100,13 +185,26 @@ void Systrace::begin_body(int fd, int pid, const char* name) noexcept { * it is impossible for name_len to be < 0 if len >= sizeof(buf). */ \ int name_len = strlen(name) - (len - sizeof(buf)) - 1; \ /* Truncate the name to make the message fit. */ \ - slog.e << "Truncated name in " << __FUNCTION__ << ": " << name << io::endl; \ len = snprintf(buf, sizeof(buf), format_begin "%.*s" format_end, pid, \ name_len, name, value); \ } \ write(fd, buf, len); \ } +void Systrace::begin_body(int fd, int pid, const char* name) noexcept { + char buf[ATRACE_MESSAGE_LENGTH]; + ssize_t len = snprintf(buf, sizeof(buf), "B|%d|%s", pid, name); + if (len >= sizeof(buf)) { + len = sizeof(buf) - 1; + } + write(fd, buf, size_t(len)); +} + +void Systrace::end_body(int fd, int pid) noexcept { + const char END_TAG = 'E'; + write(fd, &END_TAG, 1); +} + void Systrace::async_begin_body(int fd, int pid, const char* name, int32_t cookie) noexcept { WRITE_MSG("S|%d|", "|%" PRId32, pid, name, cookie); } @@ -115,26 +213,10 @@ void Systrace::async_end_body(int fd, int pid, const char* name, int32_t cookie) WRITE_MSG("F|%d|", "|%" PRId32, pid, name, cookie); } -void Systrace::int_body(int fd, int pid, const char* name, int32_t value) noexcept { - WRITE_MSG("C|%d|", "|%" PRId32, pid, name, value); -} - void Systrace::int64_body(int fd, int pid, const char* name, int64_t value) noexcept { WRITE_MSG("C|%d|", "|%" PRId64, pid, name, value); } -// ------------------------------------------------------------------------------------------------ - -void Systrace::init(uint32_t tag) noexcept { - // must be called first - mIsTracingEnabled = isTracingEnabled(tag); - - // these are now initialized - mMarkerFd = sMarkerFd; - - mPid = getpid(); -} - } // namespace details } // namespace utils From 5356f650401319673a208270f01dcf344081a6a9 Mon Sep 17 00:00:00 2001 From: Mathias Agopian Date: Thu, 3 Jun 2021 10:48:16 -0700 Subject: [PATCH 17/24] use "debug" namespace for system properties "filament.perfcounters" is renamed to "debug.filament.perfcounters", because otherwise it can't be set by users on recent Android versions. --- filament/backend/src/CommandStream.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/filament/backend/src/CommandStream.cpp b/filament/backend/src/CommandStream.cpp index 7fbb5b23242..2621e6d496f 100644 --- a/filament/backend/src/CommandStream.cpp +++ b/filament/backend/src/CommandStream.cpp @@ -66,7 +66,7 @@ CommandStream::CommandStream(Driver& driver, CircularBuffer& buffer) noexcept { #ifdef ANDROID char property[PROP_VALUE_MAX]; - __system_property_get("filament.perfcounters", property); + __system_property_get("debug.filament.perfcounters", property); mUsePerformanceCounter = bool(atoi(property)); #endif } From e297f65d8cdf203daaf9c35f766672a42eb642e8 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Thu, 3 Jun 2021 13:22:05 -0700 Subject: [PATCH 18/24] getLight() should initialize all fields. Fixes #4014. --- shaders/src/light_punctual.fs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shaders/src/light_punctual.fs b/shaders/src/light_punctual.fs index 841213da530..b59ef13d1a9 100644 --- a/shaders/src/light_punctual.fs +++ b/shaders/src/light_punctual.fs @@ -133,6 +133,10 @@ Light getLight(const uint index) { light.attenuation = getDistanceAttenuation(posToLight, positionFalloff.w); light.NoL = saturate(dot(shading_normal, light.l)); light.worldPosition = positionFalloff.xyz; + light.castsShadows = false; + light.contactShadows = false; + light.shadowIndex = 0u; + light.shadowLayer = 0u; uint type = floatBitsToUint(scaleOffsetShadowType.w); if (type == LIGHT_TYPE_SPOT) { From d53f7ecd6a3d5e05f1afb6437b0b5e5a97f00b13 Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Mon, 7 Jun 2021 11:14:43 -0700 Subject: [PATCH 19/24] Release Filament 1.10.2 --- README.md | 4 ++-- android/gradle.properties | 2 +- ios/CocoaPods/Filament.podspec | 4 ++-- web/filament-js/package.json | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 8e9dd02d8ce..47f2b550eb8 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ repositories { } dependencies { - implementation 'com.google.android.filament:filament-android:1.10.1' + implementation 'com.google.android.filament:filament-android:1.10.2' } ``` @@ -63,7 +63,7 @@ A much smaller alternative to `filamat-android` that can only generate OpenGL sh iOS projects can use CocoaPods to install the latest release: ``` -pod 'Filament', '~> 1.10.1' +pod 'Filament', '~> 1.10.2' ``` ### Snapshots diff --git a/android/gradle.properties b/android/gradle.properties index 6598c64239a..eba447bb219 100644 --- a/android/gradle.properties +++ b/android/gradle.properties @@ -1,5 +1,5 @@ GROUP=com.google.android.filament -VERSION_NAME=1.10.1 +VERSION_NAME=1.10.2 POM_DESCRIPTION=Real-time physically based rendering engine for Android. diff --git a/ios/CocoaPods/Filament.podspec b/ios/CocoaPods/Filament.podspec index 1e6d01c6d8f..f42ab416c1d 100644 --- a/ios/CocoaPods/Filament.podspec +++ b/ios/CocoaPods/Filament.podspec @@ -1,12 +1,12 @@ Pod::Spec.new do |spec| spec.name = "Filament" - spec.version = "1.10.1" + spec.version = "1.10.2" spec.license = { :type => "Apache 2.0", :file => "LICENSE" } spec.homepage = "https://google.github.io/filament" spec.authors = "Google LLC." spec.summary = "Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WASM/WebGL." spec.platform = :ios, "11.0" - spec.source = { :http => "https://github.com/google/filament/releases/download/v1.10.1/filament-v1.10.1-ios.tgz" } + spec.source = { :http => "https://github.com/google/filament/releases/download/v1.10.2/filament-v1.10.2-ios.tgz" } # Fix linking error with Xcode 12; we do not yet support the simulator on Apple silicon. spec.pod_target_xcconfig = { diff --git a/web/filament-js/package.json b/web/filament-js/package.json index 4e25eac56e6..2ca1f45c19d 100644 --- a/web/filament-js/package.json +++ b/web/filament-js/package.json @@ -1,6 +1,6 @@ { "name": "filament", - "version": "1.10.1", + "version": "1.10.2", "description": "Real-time physically based rendering engine", "main": "filament.js", "module": "filament.js", From 02a8cffb18562625a4ab52d7cb63d24c8a7cbaba Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Mon, 7 Jun 2021 11:14:17 -0700 Subject: [PATCH 20/24] Update RELEASE_NOTES for 1.10.2 --- RELEASE_NOTES.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 3133372e939..9e86ad686c4 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -3,11 +3,19 @@ This file contains one line summaries of commits that are worthy of mentioning in release notes. A new header is inserted each time a *tag* is created. -## v1.10.3 (currently main branch) +## v1.10.4 (currently main branch) + +## v1.10.3 ## v1.10.2 +- Vulkan: validation and diagnostic improvements +- engine: improvements for scenes with many renderables. - gltfio: added support for `KHR_materials_ior`. +- java: Add bindings for `IBLPrefilterContext`. +- java: add `KTXLoader.getSphericalHarmonics` JNI binding +- libimage: fix, respect sRGB option for compressed formats. +- sample-gltf-viewer: fix lifetime cycle for RemoteServer. ## v1.10.1 From fc06298ed4feafd5112ff21c1375456010c39fac Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Mon, 7 Jun 2021 11:22:04 -0700 Subject: [PATCH 21/24] Bump version to 1.10.3 --- README.md | 4 ++-- android/gradle.properties | 2 +- ios/CocoaPods/Filament.podspec | 4 ++-- web/filament-js/package.json | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 47f2b550eb8..1b8805c0a5d 100644 --- a/README.md +++ b/README.md @@ -31,7 +31,7 @@ repositories { } dependencies { - implementation 'com.google.android.filament:filament-android:1.10.2' + implementation 'com.google.android.filament:filament-android:1.10.3' } ``` @@ -63,7 +63,7 @@ A much smaller alternative to `filamat-android` that can only generate OpenGL sh iOS projects can use CocoaPods to install the latest release: ``` -pod 'Filament', '~> 1.10.2' +pod 'Filament', '~> 1.10.3' ``` ### Snapshots diff --git a/android/gradle.properties b/android/gradle.properties index eba447bb219..d63f0ca955d 100644 --- a/android/gradle.properties +++ b/android/gradle.properties @@ -1,5 +1,5 @@ GROUP=com.google.android.filament -VERSION_NAME=1.10.2 +VERSION_NAME=1.10.3 POM_DESCRIPTION=Real-time physically based rendering engine for Android. diff --git a/ios/CocoaPods/Filament.podspec b/ios/CocoaPods/Filament.podspec index f42ab416c1d..10d85fa77ce 100644 --- a/ios/CocoaPods/Filament.podspec +++ b/ios/CocoaPods/Filament.podspec @@ -1,12 +1,12 @@ Pod::Spec.new do |spec| spec.name = "Filament" - spec.version = "1.10.2" + spec.version = "1.10.3" spec.license = { :type => "Apache 2.0", :file => "LICENSE" } spec.homepage = "https://google.github.io/filament" spec.authors = "Google LLC." spec.summary = "Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WASM/WebGL." spec.platform = :ios, "11.0" - spec.source = { :http => "https://github.com/google/filament/releases/download/v1.10.2/filament-v1.10.2-ios.tgz" } + spec.source = { :http => "https://github.com/google/filament/releases/download/v1.10.3/filament-v1.10.3-ios.tgz" } # Fix linking error with Xcode 12; we do not yet support the simulator on Apple silicon. spec.pod_target_xcconfig = { diff --git a/web/filament-js/package.json b/web/filament-js/package.json index 2ca1f45c19d..e4b5dd7aa24 100644 --- a/web/filament-js/package.json +++ b/web/filament-js/package.json @@ -1,6 +1,6 @@ { "name": "filament", - "version": "1.10.2", + "version": "1.10.3", "description": "Real-time physically based rendering engine", "main": "filament.js", "module": "filament.js", From c1a0e61e8e4423fa442fde7e2e858bda69499f32 Mon Sep 17 00:00:00 2001 From: Philip Rideout Date: Mon, 7 Jun 2021 13:17:53 -0700 Subject: [PATCH 22/24] Fix FixedCapacityVector destructor. std::allocator::deallocate() expects the same value that was given during allocate(). Interestingly, this bug did not manifest any issues (even with ASAN) on some platforms. --- libs/utils/include/utils/FixedCapacityVector.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/utils/include/utils/FixedCapacityVector.h b/libs/utils/include/utils/FixedCapacityVector.h index 7e2150eb2b6..853825bb29c 100644 --- a/libs/utils/include/utils/FixedCapacityVector.h +++ b/libs/utils/include/utils/FixedCapacityVector.h @@ -108,7 +108,7 @@ class UTILS_PUBLIC FixedCapacityVector { ~FixedCapacityVector() noexcept { destroy(begin(), end()); - allocator().deallocate(data(), size()); + allocator().deallocate(data(), capacity()); } FixedCapacityVector& operator=(FixedCapacityVector const& rhs) { From 6b3c1179bc4a6a4fba4621e9d1ab54a70d1fec21 Mon Sep 17 00:00:00 2001 From: Ben Doherty Date: Tue, 8 Jun 2021 10:32:49 -0700 Subject: [PATCH 23/24] Include sample-gltf-viewer with Android releases (#4099) --- .github/workflows/release.yml | 16 ++++++++- build.sh | 65 ++++++++++++++++++++++++++++++++++- build/android/build.sh | 8 ++++- 3 files changed, 86 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b726d8a85c8..7977232ed47 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -96,6 +96,20 @@ jobs: - name: Run build script run: | cd build/android && printf "y" | ./build.sh release + - name: Sign sample-gltf-viewer + run: | + echo "${APK_KEYSTORE_BASE64}" > filament.jks.base64 + base64 --decode filament.jks.base64 > filament.jks + BUILD_TOOLS_VERSION=$(ls ${ANDROID_HOME}/build-tools | sort -V | tail -n 1) + APKSIGNER=${ANDROID_HOME}/build-tools/${BUILD_TOOLS_VERSION}/apksigner + IN_FILE="out/sample-gltf-viewer-release.apk" + OUT_FILE="out/filament-gltf-viewer-${TAG}-android.apk" + ${APKSIGNER} sign --ks filament.jks --key-pass=pass:${APK_KEYSTORE_PASS} --ks-pass=pass:${APK_KEYSTORE_PASS} --in ${IN_FILE} --out ${OUT_FILE} + rm "${IN_FILE}" + env: + TAG: ${{ steps.git_ref.outputs.tag }} + APK_KEYSTORE_BASE64: ${{ secrets.APK_KEYSTORE_BASE64 }} + APK_KEYSTORE_PASS: ${{ secrets.APK_KEYSTORE_PASS }} - name: Upload release assets run: | pip3 install setuptools @@ -107,7 +121,7 @@ jobs: mv out/gltfio-android-lite-release.aar out/gltfio-${TAG}-lite-android.aar mv out/filament-utils-android-release.aar out/filament-utils-${TAG}-android.aar mv out/filament-utils-android-lite-release.aar out/filament-utils-${TAG}-lite-android.aar - python3 build/common/upload-assets.py ${TAG} out/*.aar + python3 build/common/upload-assets.py ${TAG} out/*.aar out/*.apk env: TAG: ${{ steps.git_ref.outputs.tag }} GITHUB_API_KEY: ${{ secrets.GITHUB_API_KEY }} diff --git a/build.sh b/build.sh index b00f73d8640..03a2cac1024 100755 --- a/build.sh +++ b/build.sh @@ -52,6 +52,10 @@ function print_help { echo " For macOS, this builds universal binaries for both Apple silicon and Intel-based Macs." echo " -w" echo " Build Web documents (compiles .md.html files to .html)." + echo " -k sample1,sample2,..." + echo " When building for Android, also build select sample APKs." + echo " sampleN is an Android sample, e.g., sample-gltf-viewer." + echo " This automatically performs a partial desktop build and install." echo "" echo "Build types:" echo " release" @@ -141,6 +145,9 @@ ISSUE_CMAKE_ALWAYS=false ISSUE_WEB_DOCS=false +ANDROID_SAMPLES=() +BUILD_ANDROID_SAMPLES=false + RUN_TESTS=false FILAMENT_ENABLE_JAVA=ON @@ -406,6 +413,24 @@ function build_android { build_desktop "${MOBILE_HOST_TOOLS}" + # When building the samples, we need to partially "install" the host tools so Gradle can see + # them. + if [[ "${BUILD_ANDROID_SAMPLES}" == "true" ]]; then + if [[ "${ISSUE_DEBUG_BUILD}" == "true" ]]; then + mkdir -p out/debug/filament/bin + for tool in ${MOBILE_HOST_TOOLS}; do + cp out/cmake-debug/tools/${tool}/${tool} out/debug/filament/bin/ + done + fi + + if [[ "${ISSUE_RELEASE_BUILD}" == "true" ]]; then + mkdir -p out/release/filament/bin + for tool in ${MOBILE_HOST_TOOLS}; do + cp out/cmake-release/tools/${tool}/${tool} out/release/filament/bin/ + done + fi + fi + INSTALL_COMMAND=${old_install_command} if [[ "${ABI_ARM64_V8A}" == "true" ]]; then @@ -445,6 +470,15 @@ function build_android { -Pfilament_abis=${ABI_GRADLE_OPTION} \ :filamat-android:assembleDebug + if [[ "${BUILD_ANDROID_SAMPLES}" == "true" ]]; then + for sample in ${ANDROID_SAMPLES}; do + ./gradlew \ + -Pfilament_dist_dir=../out/android-debug/filament \ + -Pfilament_abis=${ABI_GRADLE_OPTION} \ + :samples:${sample}:assembleDebug + done + fi + if [[ "${INSTALL_COMMAND}" ]]; then echo "Installing out/filamat-android-debug.aar..." cp filamat-android/build/outputs/aar/filamat-android-lite-debug.aar ../out/ @@ -460,6 +494,14 @@ function build_android { echo "Installing out/filament-utils-android-debug.aar..." cp filament-utils-android/build/outputs/aar/filament-utils-android-lite-debug.aar ../out/ cp filament-utils-android/build/outputs/aar/filament-utils-android-full-debug.aar ../out/filament-utils-android-debug.aar + + if [[ "${BUILD_ANDROID_SAMPLES}" == "true" ]]; then + for sample in ${ANDROID_SAMPLES}; do + echo "Installing out/${sample}-debug.apk" + cp samples/${sample}/build/outputs/apk/debug/${sample}-debug-unsigned.apk \ + ../out/${sample}-debug.apk + done + fi fi fi @@ -477,6 +519,15 @@ function build_android { -Pfilament_abis=${ABI_GRADLE_OPTION} \ :filamat-android:assembleRelease + if [[ "${BUILD_ANDROID_SAMPLES}" == "true" ]]; then + for sample in ${ANDROID_SAMPLES}; do + ./gradlew \ + -Pfilament_dist_dir=../out/android-release/filament \ + -Pfilament_abis=${ABI_GRADLE_OPTION} \ + :samples:${sample}:assembleRelease + done + fi + if [[ "${INSTALL_COMMAND}" ]]; then echo "Installing out/filamat-android-release.aar..." cp filamat-android/build/outputs/aar/filamat-android-lite-release.aar ../out/ @@ -492,6 +543,14 @@ function build_android { echo "Installing out/filament-utils-android-release.aar..." cp filament-utils-android/build/outputs/aar/filament-utils-android-lite-release.aar ../out/ cp filament-utils-android/build/outputs/aar/filament-utils-android-full-release.aar ../out/filament-utils-android-release.aar + + if [[ "${BUILD_ANDROID_SAMPLES}" == "true" ]]; then + for sample in ${ANDROID_SAMPLES}; do + echo "Installing out/${sample}-release.apk" + cp samples/${sample}/build/outputs/apk/release/${sample}-release-unsigned.apk \ + ../out/${sample}-release.apk + done + fi fi fi @@ -689,7 +748,7 @@ function run_tests { pushd "$(dirname "$0")" > /dev/null -while getopts ":hacCfijmp:q:uvslwtd" opt; do +while getopts ":hacCfijmp:q:uvslwtdk:" opt; do case ${opt} in h) print_help @@ -806,6 +865,10 @@ while getopts ":hacCfijmp:q:uvslwtd" opt; do w) ISSUE_WEB_DOCS=true ;; + k) + BUILD_ANDROID_SAMPLES=true + ANDROID_SAMPLES=$(echo "${OPTARG}" | tr ',' '\n') + ;; \?) echo "Invalid option: -${OPTARG}" >&2 echo "" diff --git a/build/android/build.sh b/build/android/build.sh index c4b798dc61f..996eeeda21e 100755 --- a/build/android/build.sh +++ b/build/android/build.sh @@ -59,5 +59,11 @@ if [[ "$TARGET" == "presubmit" ]]; then ANDROID_ABIS="-q arm64-v8a,x86" fi +# Build the Android sample-gltf-viewer APK during release. +BUILD_SAMPLES= +if [[ "$TARGET" == "release" ]]; then + BUILD_SAMPLES="-k sample-gltf-viewer" +fi + pushd `dirname $0`/../.. > /dev/null -FILAMENT_NDK_VERSION=${FILAMENT_NDK_VERSION} ./build.sh -p android $ANDROID_ABIS -c $GENERATE_ARCHIVES $BUILD_DEBUG $BUILD_RELEASE +FILAMENT_NDK_VERSION=${FILAMENT_NDK_VERSION} ./build.sh -p android $ANDROID_ABIS -c $BUILD_SAMPLES $GENERATE_ARCHIVES $BUILD_DEBUG $BUILD_RELEASE From c4259b559800fcb82c6b57607adca876477de869 Mon Sep 17 00:00:00 2001 From: Benjamin Doherty Date: Mon, 14 Jun 2021 11:02:45 -0600 Subject: [PATCH 24/24] Update RELEASE_NOTES for 1.10.3 --- RELEASE_NOTES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 9e86ad686c4..fec357edc60 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,12 @@ A new header is inserted each time a *tag* is created. ## v1.10.3 +- android: use `debug.filament.backend` system property to select the desired backend. +- engine: fix `LightManager::getFalloff`. +- gltfio: fix crash with non-triangles. +- macOS: fix main thread checker warnings with OpenGL. +- vulkan: fix crash on Windows machines with NVIDIA GPUs. + ## v1.10.2 - Vulkan: validation and diagnostic improvements