Refactored OpenXRSwapchainImpl to use two private objects that instantiate a swapchain, instead of instantiating two swapchains directly. Added handling of runtime failures in xrAcquireSwapchain(), xrWaitSwapchain(), and xrReleaseSwapchain().

apps/openmw/mwvr/openxrmanagerimpl.hpp

@@ -24,8 +24,8 @@ namespace MWVR
 #define CHK_STRINGIFY(x) #x
 #define TOSTRING(x) CHK_STRINGIFY(x)
 #define FILE_AND_LINE __FILE__ ":" TOSTRING(__LINE__)
-#define CHECK_XRCMD(cmd) CheckXrResult(cmd, #cmd, FILE_AND_LINE);
+#define CHECK_XRCMD(cmd) CheckXrResult(cmd, #cmd, FILE_AND_LINE)
-#define CHECK_XRRESULT(res, cmdStr) CheckXrResult(res, cmdStr, FILE_AND_LINE);
+#define CHECK_XRRESULT(res, cmdStr) CheckXrResult(res, cmdStr, FILE_AND_LINE)
     XrResult CheckXrResult(XrResult res, const char* originator = nullptr, const char* sourceLocation = nullptr);
     std::string XrResultString(XrResult res);
 

apps/openmw/mwvr/openxrswapchainimpl.cpp

@@ -25,19 +25,136 @@
 
 namespace MWVR {
     OpenXRSwapchainImpl::OpenXRSwapchainImpl(osg::ref_ptr<osg::State> state, SwapchainConfig config)
-        : mSwapchainColorBuffers()
+        : mConfig(config)
-        , mSwapchainDepthBuffers()
+    {
+        if (mConfig.selectedWidth <= 0)
+            throw std::invalid_argument("Width must be a positive integer");
+        if (mConfig.selectedHeight <= 0)
+            throw std::invalid_argument("Height must be a positive integer");
+        if (mConfig.selectedSamples <= 0)
+            throw std::invalid_argument("Samples must be a positive integer");
+
+        mSwapchain.reset(new SwapchainPrivate(state, mConfig, SwapchainPrivate::Use::COLOR));
+        mConfig.selectedSamples = mSwapchain->samples();
+
+        auto* xr = Environment::get().getManager();
+
+        if (xr->xrExtensionIsEnabled(XR_KHR_COMPOSITION_LAYER_DEPTH_EXTENSION_NAME))
+        {
+            try
+            {
+                mSwapchainDepth.reset(new SwapchainPrivate(state, mConfig, SwapchainPrivate::Use::DEPTH));
+            }
+            catch (std::exception& e)
+            {
+                Log(Debug::Warning) << "XR_KHR_composition_layer_depth was enabled but creating depth swapchain failed: " << e.what();
+                mSwapchainDepth = nullptr;
+            }
+        }
+
+        uint32_t imageCount = mSwapchain->count();
+        for (uint32_t i = 0; i < imageCount; i++)
+        {
+            uint32_t colorBuffer = mSwapchain->bufferAt(i);
+            uint32_t depthBuffer = mSwapchainDepth ? mSwapchainDepth->bufferAt(i) : 0;
+            mRenderBuffers.emplace_back(new VRFramebuffer(state, mSwapchain->width(), mSwapchain->height(), mSwapchain->samples(), colorBuffer, depthBuffer));
+        }
+    }
+
+    OpenXRSwapchainImpl::~OpenXRSwapchainImpl()
+    {
+    }
+
+    VRFramebuffer* OpenXRSwapchainImpl::renderBuffer() const
+    {
+        checkAcquired();
+        // Note that I am trusting that the openxr runtime won't diverge the indexes of the depth and color swapchains so long as these are always acquired together.
+        // If some dumb ass implementation decides to violate this we'll just have to work around that if it actually happens.
+        return mRenderBuffers[mSwapchain->acuiredIndex()].get();
+    }
+
+    uint32_t OpenXRSwapchainImpl::acquiredColorTexture() const
+    {
+        checkAcquired();
+        return mSwapchain->acuiredBuffer();
+    }
+
+    uint32_t OpenXRSwapchainImpl::acquiredDepthTexture() const
+    {
+        if (mSwapchainDepth)
+        {
+            checkAcquired();
+            return mSwapchainDepth->acuiredBuffer();
+        }
+        return 0;
+    }
+
+    bool OpenXRSwapchainImpl::isAcquired() const
+    {
+        return mFormallyAcquired;
+    }
+
+    void OpenXRSwapchainImpl::beginFrame(osg::GraphicsContext* gc)
+    {
+        acquire();
+        renderBuffer()->bindFramebuffer(gc, GL_FRAMEBUFFER_EXT);
+    }
+
+    int swapCount = 0;
+
+    void OpenXRSwapchainImpl::endFrame(osg::GraphicsContext* gc)
+    {
+        checkAcquired();
+        release();
+    }
+
+    void OpenXRSwapchainImpl::acquire()
+    {
+        if (isAcquired())
+            throw std::logic_error("Trying to acquire already acquired swapchain");
+
+        if (!mShouldRelease)
+        {
+            mSwapchain->acquire();
+            mShouldRelease = mSwapchain->isAcquired();
+            if (mSwapchainDepth && mSwapchain->isAcquired())
+            {
+                mSwapchainDepth->acquire();
+                mShouldRelease = mSwapchainDepth->isAcquired();
+            }
+        }
+
+        mFormallyAcquired = true;
+    }
+
+    void OpenXRSwapchainImpl::release()
+    {
+        if (mShouldRelease)
+        {
+            mSwapchain->release();
+            mShouldRelease = mSwapchain->isAcquired();
+            if (mSwapchainDepth)
+            {
+                mSwapchainDepth->release();
+                mShouldRelease = mSwapchainDepth->isAcquired();
+            }
+        }
+
+        mFormallyAcquired = false;
+    }
+
+    void OpenXRSwapchainImpl::checkAcquired() const
+    {
+        if (!isAcquired())
+            throw std::logic_error("Swapchain must be acquired before use. Call between OpenXRSwapchain::beginFrame() and OpenXRSwapchain::endFrame()");
+    }
+
+    OpenXRSwapchainImpl::SwapchainPrivate::SwapchainPrivate(osg::ref_ptr<osg::State> state, SwapchainConfig config, Use use)
+        : mBuffers()
         , mWidth(config.selectedWidth)
         , mHeight(config.selectedHeight)
         , mSamples(config.selectedSamples)
     {
-        if (mWidth <= 0)
-            throw std::invalid_argument("Width must be a positive integer");
-        if (mHeight <= 0)
-            throw std::invalid_argument("Height must be a positive integer");
-        if (mSamples <= 0)
-            throw std::invalid_argument("Samples must be a positive integer");
-
         auto* xr = Environment::get().getManager();
 
         // Select a swapchain format.
@@ -54,37 +171,31 @@ namespace MWVR {
             //GL_RGBA16,
             //0x881A // GL_RGBA16F
         };
+        constexpr int64_t RequestedDepthSwapchainFormats[] = {
+            GL_DEPTH_COMPONENT24,
+            GL_DEPTH_COMPONENT32F,
+        };
 
-        auto swapchainFormatIt =
+        auto colorFormatIt =
             std::find_first_of(swapchainFormats.begin(), swapchainFormats.end(), std::begin(RequestedColorSwapchainFormats),
                 std::end(RequestedColorSwapchainFormats));
-        if (swapchainFormatIt == swapchainFormats.end()) {
+        auto depthFormatIt =
-            throw std::runtime_error("Swapchain color format not supported");
+            std::find_first_of(swapchainFormats.begin(), swapchainFormats.end(), std::begin(RequestedDepthSwapchainFormats),
+                std::end(RequestedDepthSwapchainFormats));
+
+        auto formatIt = use == Use::COLOR ? colorFormatIt : depthFormatIt;
+        std::string typeString = use == Use::COLOR ? "color" : "depth";
+
+        if (formatIt == swapchainFormats.end()) {
+            throw std::runtime_error(std::string("Swapchain ") + typeString + " format not supported");
         }
-        mSwapchainColorFormat = *swapchainFormatIt;
+
-        Log(Debug::Verbose) << "Selected color format: " << std::dec << mSwapchainColorFormat << " (" << std::hex << mSwapchainColorFormat << ")" << std::dec;
+        mFormat = *formatIt;
+        Log(Debug::Verbose) << "Selected " << typeString << " format: " << std::dec << mFormat << " (" << std::hex << mFormat << ")" << std::dec;
 
         if (xr->xrExtensionIsEnabled(XR_KHR_COMPOSITION_LAYER_DEPTH_EXTENSION_NAME))
         {
-            // Find supported depth swapchain format.
+            // TODO
-            constexpr int64_t RequestedDepthSwapchainFormats[] = {
-                GL_DEPTH_COMPONENT24,
-                GL_DEPTH_COMPONENT32F,
-            };
-
-            swapchainFormatIt =
-                std::find_first_of(swapchainFormats.begin(), swapchainFormats.end(), std::begin(RequestedDepthSwapchainFormats),
-                    std::end(RequestedDepthSwapchainFormats));
-            if (swapchainFormatIt == swapchainFormats.end()) {
-                mHaveDepthSwapchain = false;
-                Log(Debug::Warning) << "OpenXR extension " << XR_KHR_COMPOSITION_LAYER_DEPTH_EXTENSION_NAME << " enabled, but no depth formats were found";
-            }
-            else
-            {
-                mSwapchainDepthFormat = *swapchainFormatIt;
-                mHaveDepthSwapchain = true;
-                Log(Debug::Verbose) << "Selected depth format: " << std::dec << mSwapchainDepthFormat << " (" << std::hex << mSwapchainDepthFormat << ")" << std::dec;
-            }
         }
 
         XrSwapchainCreateInfo swapchainCreateInfo{ XR_TYPE_SWAPCHAIN_CREATE_INFO };
@@ -98,7 +209,7 @@ namespace MWVR {
         {
             Log(Debug::Verbose) << "Creating swapchain with dimensions Width=" << mWidth << " Heigh=" << mHeight << " SampleCount=" << mSamples;
             // First create the swapchain of color buffers.
-            swapchainCreateInfo.format = mSwapchainColorFormat;
+            swapchainCreateInfo.format = mFormat;
             swapchainCreateInfo.sampleCount = mSamples;
             swapchainCreateInfo.usageFlags = XR_SWAPCHAIN_USAGE_SAMPLED_BIT | XR_SWAPCHAIN_USAGE_COLOR_ATTACHMENT_BIT;
             auto res = xrCreateSwapchain(xr->impl().xrSession(), &swapchainCreateInfo, &mSwapchain);
@@ -115,126 +226,72 @@ namespace MWVR {
 
         uint32_t imageCount = 0;
         CHECK_XRCMD(xrEnumerateSwapchainImages(mSwapchain, 0, &imageCount, nullptr));
-        mSwapchainColorBuffers.resize(imageCount, { XR_TYPE_SWAPCHAIN_IMAGE_OPENGL_KHR });
+        mBuffers.resize(imageCount, { XR_TYPE_SWAPCHAIN_IMAGE_OPENGL_KHR });
-        CHECK_XRCMD(xrEnumerateSwapchainImages(mSwapchain, imageCount, &imageCount, reinterpret_cast<XrSwapchainImageBaseHeader*>(mSwapchainColorBuffers.data())));
+        CHECK_XRCMD(xrEnumerateSwapchainImages(mSwapchain, imageCount, &imageCount, reinterpret_cast<XrSwapchainImageBaseHeader*>(mBuffers.data())));
         mSubImage.swapchain = mSwapchain;
         mSubImage.imageRect.offset = { 0, 0 };
         mSubImage.imageRect.extent = { mWidth, mHeight };
-
-        if (mHaveDepthSwapchain)
-        {
-            // Now create the swapchain of depth buffers if applicable
-            if (mHaveDepthSwapchain)
-            {
-                swapchainCreateInfo.format = mSwapchainDepthFormat;
-                swapchainCreateInfo.sampleCount = mSamples;
-                swapchainCreateInfo.usageFlags = XR_SWAPCHAIN_USAGE_SAMPLED_BIT | XR_SWAPCHAIN_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT;
-                auto res = xrCreateSwapchain(xr->impl().xrSession(), &swapchainCreateInfo, &mSwapchainDepth);
-                if (!XR_SUCCEEDED(res))
-                    throw std::runtime_error(XrResultString(res));
-                VrDebug::setName(mSwapchainDepth, "OpenMW XR Depth Swapchain " + config.name);
-            }
-            CHECK_XRCMD(xrEnumerateSwapchainImages(mSwapchainDepth, 0, &imageCount, nullptr));
-            mSwapchainDepthBuffers.resize(imageCount, { XR_TYPE_SWAPCHAIN_IMAGE_OPENGL_KHR });
-            CHECK_XRCMD(xrEnumerateSwapchainImages(mSwapchainDepth, imageCount, &imageCount, reinterpret_cast<XrSwapchainImageBaseHeader*>(mSwapchainDepthBuffers.data())));
-            mSubImageDepth.swapchain = mSwapchainDepth;
-            mSubImageDepth.imageRect.offset = { 0, 0 };
-            mSubImageDepth.imageRect.extent = { mWidth, mHeight };
-        }
-
-        for (unsigned i = 0; i < imageCount; i++)
-        {
-            uint32_t colorBuffer = mSwapchainColorBuffers[i].image;
-            uint32_t depthBuffer = mHaveDepthSwapchain ? mSwapchainDepthBuffers[i].image : 0;
-            mRenderBuffers.emplace_back(new VRFramebuffer(state, mWidth, mHeight, mSamples, colorBuffer, depthBuffer));
-        }
     }
 
-    OpenXRSwapchainImpl::~OpenXRSwapchainImpl()
+    OpenXRSwapchainImpl::SwapchainPrivate::~SwapchainPrivate()
     {
         if (mSwapchain)
             CHECK_XRCMD(xrDestroySwapchain(mSwapchain));
     }
 
-    VRFramebuffer* OpenXRSwapchainImpl::renderBuffer() const
+    uint32_t OpenXRSwapchainImpl::SwapchainPrivate::bufferAt(uint32_t index) const
+    {
+        return mBuffers[index].image;
+    }
+
+    uint32_t OpenXRSwapchainImpl::SwapchainPrivate::count() const
+    {
+        return mBuffers.size();
+    }
+
+    uint32_t OpenXRSwapchainImpl::SwapchainPrivate::acuiredBuffer() const
     {
         checkAcquired();
-        return mRenderBuffers[mAcquiredImageIndex].get();
+        return mBuffers[mAcquiredIndex].image;
     }
-
+    bool OpenXRSwapchainImpl::SwapchainPrivate::isAcquired() const
-    uint32_t OpenXRSwapchainImpl::acquiredColorTexture() const
     {
-        checkAcquired();
+        return mIsReady;
-        return mSwapchainColorBuffers[mAcquiredImageIndex].image;
     }
 
-    uint32_t OpenXRSwapchainImpl::acquiredDepthTexture() const
+    void OpenXRSwapchainImpl::SwapchainPrivate::acquire()
-    {
-        checkAcquired();
-        return mSwapchainColorBuffers[mAcquiredImageIndex].image;
-    }
-
-    bool OpenXRSwapchainImpl::isAcquired() const
-    {
-        return mIsAcquired;
-    }
-
-    void OpenXRSwapchainImpl::beginFrame(osg::GraphicsContext* gc)
-    {
-        if (isAcquired())
-            throw std::logic_error("Trying to acquire already acquired swapchain");
-        acquire(gc);
-        renderBuffer()->bindFramebuffer(gc, GL_FRAMEBUFFER_EXT);
-    }
-
-    int swapCount = 0;
-
-    void OpenXRSwapchainImpl::endFrame(osg::GraphicsContext* gc)
-    {
-        checkAcquired();
-        release(gc);
-    }
-
-    void OpenXRSwapchainImpl::acquire(osg::GraphicsContext*)
     {
         auto xr = Environment::get().getManager();
         XrSwapchainImageAcquireInfo acquireInfo{ XR_TYPE_SWAPCHAIN_IMAGE_ACQUIRE_INFO };
         XrSwapchainImageWaitInfo waitInfo{ XR_TYPE_SWAPCHAIN_IMAGE_WAIT_INFO };
         waitInfo.timeout = XR_INFINITE_DURATION;
-        // I am trusting that the openxr runtime won't diverge these indices so long as these are always called together.
+        if (!mIsIndexAcquired)
-        // If some dumb ass implementation decides to violate this we'll just have to work around that if it actually happens.
-        CHECK_XRCMD(xrAcquireSwapchainImage(mSwapchain, &acquireInfo, &mAcquiredImageIndex));
-        CHECK_XRCMD(xrWaitSwapchainImage(mSwapchain, &waitInfo));
-        xr->xrResourceAcquired();
-
-        if (mHaveDepthSwapchain)
         {
-            uint32_t depthIndex = 0;
+            mIsIndexAcquired = XR_SUCCEEDED(CHECK_XRCMD(xrAcquireSwapchainImage(mSwapchain, &acquireInfo, &mAcquiredIndex)));
-            CHECK_XRCMD(xrAcquireSwapchainImage(mSwapchainDepth, &acquireInfo, &depthIndex));
+            if (mIsIndexAcquired)
-            if (depthIndex != mAcquiredImageIndex)
+                xr->xrResourceAcquired();
-                Log(Debug::Warning) << "Depth and color indices diverged";
+        }
-            CHECK_XRCMD(xrWaitSwapchainImage(mSwapchainDepth, &waitInfo));
+        if (mIsIndexAcquired && !mIsReady)
-            xr->xrResourceAcquired();
+        {
+            mIsReady = XR_SUCCEEDED(CHECK_XRCMD(xrWaitSwapchainImage(mSwapchain, &waitInfo)));
         }
-
-        mIsAcquired = true;
     }
-
+    void OpenXRSwapchainImpl::SwapchainPrivate::release()
-    void OpenXRSwapchainImpl::release(osg::GraphicsContext*)
     {
         auto xr = Environment::get().getManager();
-        mIsAcquired = false;
 
         XrSwapchainImageReleaseInfo releaseInfo{ XR_TYPE_SWAPCHAIN_IMAGE_RELEASE_INFO };
-        CHECK_XRCMD(xrReleaseSwapchainImage(mSwapchain, &releaseInfo));
+        if (mIsReady)
-        xr->xrResourceReleased();
-        if (mHaveDepthSwapchain)
         {
-            CHECK_XRCMD(xrReleaseSwapchainImage(mSwapchainDepth, &releaseInfo));
+            mIsReady = !XR_SUCCEEDED(CHECK_XRCMD(xrReleaseSwapchainImage(mSwapchain, &releaseInfo)));
-            xr->xrResourceReleased();
+            if (!mIsReady)
+            {
+                mIsIndexAcquired = false;
+                xr->xrResourceReleased();
+            }
         }
     }
-    void OpenXRSwapchainImpl::checkAcquired() const
+    void OpenXRSwapchainImpl::SwapchainPrivate::checkAcquired() const
     {
         if (!isAcquired())
             throw std::logic_error("Swapchain must be acquired before use. Call between OpenXRSwapchain::beginFrame() and OpenXRSwapchain::endFrame()");

apps/openmw/mwvr/openxrswapchainimpl.hpp

@@ -12,6 +12,45 @@ namespace MWVR
     /// \brief Implementation of OpenXRSwapchain
     class OpenXRSwapchainImpl
     {
+    private:
+        struct SwapchainPrivate
+        {
+            enum class Use {
+                COLOR = 0,
+                DEPTH = 1
+            };
+
+            SwapchainPrivate(osg::ref_ptr<osg::State> state, SwapchainConfig config, Use use);
+            ~SwapchainPrivate();
+
+            uint32_t bufferAt(uint32_t index) const;
+            uint32_t count() const;
+            uint32_t acuiredBuffer() const;
+            uint32_t acuiredIndex() const { return mAcquiredIndex; };
+            bool isAcquired() const;
+            XrSwapchain xrSwapchain(void) const { return mSwapchain; };
+            XrSwapchainSubImage xrSubImage(void) const { return mSubImage; };
+            int width() const { return mWidth; };
+            int height() const { return mHeight; };
+            int samples() const { return mSamples; };
+
+            void acquire();
+            void release();
+            void checkAcquired() const;
+
+        private:
+            XrSwapchain mSwapchain = XR_NULL_HANDLE;
+            XrSwapchainSubImage mSubImage{};
+            std::vector<XrSwapchainImageOpenGLKHR> mBuffers;
+            int32_t mWidth = -1;
+            int32_t mHeight = -1;
+            int32_t mSamples = -1;
+            int64_t mFormat = -1;
+            uint32_t mAcquiredIndex{ 0 };
+            bool mIsIndexAcquired{ false };
+            bool mIsReady{ false };
+        };
+
     public:
         OpenXRSwapchainImpl(osg::ref_ptr<osg::State> state, SwapchainConfig config);
         ~OpenXRSwapchainImpl();
@@ -24,40 +63,29 @@ namespace MWVR
         uint32_t acquiredDepthTexture() const;
 
         bool isAcquired() const;
-        XrSwapchain xrSwapchain(void) const { return mSwapchain; };
+        XrSwapchain xrSwapchain(void) const { return mSwapchain->xrSwapchain(); };
-        XrSwapchain xrSwapchainDepth(void) const { return mSwapchainDepth; };
+        XrSwapchain xrSwapchainDepth(void) const { return mSwapchainDepth->xrSwapchain(); };
-        XrSwapchainSubImage xrSubImage(void) const { return mSubImage; };
+        XrSwapchainSubImage xrSubImage(void) const { return mSwapchain->xrSubImage(); };
-        XrSwapchainSubImage xrSubImageDepth(void) const { return mSubImageDepth; };
+        XrSwapchainSubImage xrSubImageDepth(void) const { return mSwapchainDepth->xrSubImage(); };
-        int width() const { return mWidth; };
+        int width() const { return mConfig.selectedWidth; };
-        int height() const { return mHeight; };
+        int height() const { return mConfig.selectedHeight; };
-        int samples() const { return mSamples; };
+        int samples() const { return mConfig.selectedSamples; };
 
     protected:
         OpenXRSwapchainImpl(const OpenXRSwapchainImpl&) = delete;
         void operator=(const OpenXRSwapchainImpl&) = delete;
 
-        void acquire(osg::GraphicsContext* gc);
+        void acquire();
-        void release(osg::GraphicsContext* gc);
+        void release();
         void checkAcquired() const;
 
     private:
-        XrSwapchain mSwapchain = XR_NULL_HANDLE;
+        SwapchainConfig mConfig;
-        XrSwapchain mSwapchainDepth = XR_NULL_HANDLE;
+        std::unique_ptr<SwapchainPrivate> mSwapchain{ nullptr };
-        std::vector<XrSwapchainImageOpenGLKHR> mSwapchainColorBuffers;
+        std::unique_ptr<SwapchainPrivate> mSwapchainDepth{ nullptr };
-        std::vector<XrSwapchainImageOpenGLKHR> mSwapchainDepthBuffers;
-        XrSwapchainSubImage mSubImage{};
-        XrSwapchainSubImage mSubImageDepth{};
-        int32_t mWidth = -1;
-        int32_t mHeight = -1;
-        int32_t mSamples = -1;
-        int64_t mSwapchainColorFormat = -1;
-        int64_t mSwapchainDepthFormat = -1;
-        bool mHaveDepthSwapchain = false;
-        uint32_t mFBO = 0;
         std::vector<std::unique_ptr<VRFramebuffer> > mRenderBuffers{};
-        int mRenderBuffer{ 0 };
+        bool mFormallyAcquired{ false };
-        uint32_t mAcquiredImageIndex{ 0 };
+        bool mShouldRelease{ false };
-        bool mIsAcquired{ false };
     };
 }