Metal, fix callbacks being called only once (#7856)

filament/backend/CMakeLists.txt

@@ -417,6 +417,7 @@ if (APPLE OR LINUX)
         test/test_MissingRequiredAttributes.cpp
         test/test_ReadPixels.cpp
         test/test_BufferUpdates.cpp
+        test/test_Callbacks.cpp
         test/test_MRT.cpp
         test/test_LoadImage.cpp
         test/test_StencilBuffer.cpp

filament/backend/include/private/backend/DriverAPI.inc

@@ -144,8 +144,7 @@ DECL_DRIVER_API_N(setFrameScheduledCallback,
 DECL_DRIVER_API_N(setFrameCompletedCallback,
         backend::SwapChainHandle, sch,
         backend::CallbackHandler*, handler,
-        backend::CallbackHandler::Callback, callback,
-        void*, user)
+        utils::Invocable<void(void)>&&, callback)
 
 DECL_DRIVER_API_N(setPresentationTime,
         int64_t, monotonic_clock_ns)

filament/backend/src/metal/MetalDriver.mm

@@ -243,10 +243,10 @@ void MetalDriver::setFrameScheduledCallback(
     swapChain->setFrameScheduledCallback(handler, std::move(callback));
 }
 
-void MetalDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
-        CallbackHandler* handler, CallbackHandler::Callback callback, void* user) {
+void MetalDriver::setFrameCompletedCallback(
+        Handle<HwSwapChain> sch, CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {
     auto* swapChain = handle_cast<MetalSwapChain>(sch);
-    swapChain->setFrameCompletedCallback(handler, callback, user);
+    swapChain->setFrameCompletedCallback(handler, std::move(callback));
 }
 
 void MetalDriver::execute(std::function<void(void)> const& fn) noexcept {

filament/backend/src/metal/MetalHandles.h

@@ -75,7 +75,7 @@ public:
 
     void setFrameScheduledCallback(CallbackHandler* handler, FrameScheduledCallback&& callback);
     void setFrameCompletedCallback(
-            CallbackHandler* handler, CallbackHandler::Callback callback, void* user);
+            CallbackHandler* handler, utils::Invocable<void(void)>&& callback);
 
     // For CAMetalLayer-backed SwapChains, presents the drawable or schedules a
     // FrameScheduledCallback.
@@ -119,13 +119,12 @@ private:
     // PresentCallable object.
     struct {
         CallbackHandler* handler = nullptr;
-        FrameScheduledCallback callback = {};
+        std::shared_ptr<FrameScheduledCallback> callback = nullptr;
     } frameScheduled;
 
     struct {
         CallbackHandler* handler = nullptr;
-        CallbackHandler::Callback callback = {};
-        void* user = nullptr;
+        std::shared_ptr<utils::Invocable<void(void)>> callback = nullptr;
     } frameCompleted;
 };
 

filament/backend/src/metal/MetalHandles.mm

@@ -224,14 +224,13 @@ void MetalSwapChain::ensureDepthStencilTexture() {
 void MetalSwapChain::setFrameScheduledCallback(
         CallbackHandler* handler, FrameScheduledCallback&& callback) {
     frameScheduled.handler = handler;
-    frameScheduled.callback = std::move(callback);
+    frameScheduled.callback = std::make_shared<FrameScheduledCallback>(std::move(callback));
 }
 
-void MetalSwapChain::setFrameCompletedCallback(CallbackHandler* handler,
-        CallbackHandler::Callback callback, void* user) {
+void MetalSwapChain::setFrameCompletedCallback(
+        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {
     frameCompleted.handler = handler;
-    frameCompleted.callback = callback;
-    frameCompleted.user = user;
+    frameCompleted.callback = std::make_shared<utils::Invocable<void(void)>>(std::move(callback));
 }
 
 void MetalSwapChain::present() {
@@ -304,17 +303,17 @@ void MetalSwapChain::scheduleFrameScheduledCallback() {
     assert_invariant(drawable);
 
     struct Callback {
-        Callback(FrameScheduledCallback&& callback, id<CAMetalDrawable> drawable,
+        Callback(std::shared_ptr<FrameScheduledCallback> callback, id<CAMetalDrawable> drawable,
                 MetalDriver* driver)
-            : f(std::move(callback)), data(PresentDrawableData::create(drawable, driver)) {}
-        FrameScheduledCallback f;
+            : f(callback), data(PresentDrawableData::create(drawable, driver)) {}
+        std::shared_ptr<FrameScheduledCallback> f;
         // PresentDrawableData* is destroyed by maybePresentAndDestroyAsync() later.
         std::unique_ptr<PresentDrawableData> data;
         static void func(void* user) {
             auto* const c = reinterpret_cast<Callback*>(user);
             PresentDrawableData* presentDrawableData = c->data.release();
             PresentCallable presentCallable(presentDrawable, presentDrawableData);
-            c->f(presentCallable);
+            c->f->operator()(presentCallable);
             delete c;
         }
     };
@@ -322,7 +321,7 @@ void MetalSwapChain::scheduleFrameScheduledCallback() {
     // This callback pointer will be captured by the block. Even if the scheduled handler is never
     // called, the unique_ptr will still ensure we don't leak memory.
     __block auto callback =
-            std::make_unique<Callback>(std::move(frameScheduled.callback), drawable, context.driver);
+            std::make_unique<Callback>(frameScheduled.callback, drawable, context.driver);
 
     backend::CallbackHandler* handler = frameScheduled.handler;
     MetalDriver* driver = context.driver;
@@ -337,13 +336,25 @@ void MetalSwapChain::scheduleFrameCompletedCallback() {
         return;
     }
 
-    CallbackHandler* handler = frameCompleted.handler;
-    void* user = frameCompleted.user;
-    CallbackHandler::Callback callback = frameCompleted.callback;
+    struct Callback {
+        Callback(std::shared_ptr<utils::Invocable<void(void)>> callback) : f(callback) {}
+        std::shared_ptr<utils::Invocable<void(void)>> f;
+        static void func(void* user) {
+            auto* const c = reinterpret_cast<Callback*>(user);
+            c->f->operator()();
+            delete c;
+        }
+    };
 
+    // This callback pointer will be captured by the block. Even if the completed handler is never
+    // called, the unique_ptr will still ensure we don't leak memory.
+    __block auto callback = std::make_unique<Callback>(frameCompleted.callback);
+
+    CallbackHandler* handler = frameCompleted.handler;
     MetalDriver* driver = context.driver;
     [getPendingCommandBuffer(&context) addCompletedHandler:^(id<MTLCommandBuffer> cb) {
-        driver->scheduleCallback(handler, user, callback);
+        Callback* user = callback.release();
+        driver->scheduleCallback(handler, user, &Callback::func);
     }];
 }
 

filament/backend/src/noop/NoopDriver.cpp

@@ -59,7 +59,7 @@ void NoopDriver::setFrameScheduledCallback(Handle<HwSwapChain> sch,
 }
 
 void NoopDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
-        CallbackHandler* handler, CallbackHandler::Callback callback, void* user) {
+        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {
 
 }
 

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -3469,7 +3469,7 @@ void OpenGLDriver::setFrameScheduledCallback(Handle<HwSwapChain> sch,
 }
 
 void OpenGLDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
-        CallbackHandler* handler, CallbackHandler::Callback callback, void* user) {
+        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {
     DEBUG_MARKER()
 }
 

filament/backend/src/vulkan/VulkanDriver.cpp

@@ -398,7 +398,7 @@ void VulkanDriver::setFrameScheduledCallback(Handle<HwSwapChain> sch,
 }
 
 void VulkanDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
-        CallbackHandler* handler, CallbackHandler::Callback callback, void* user) {
+        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {
 }
 
 void VulkanDriver::setPresentationTime(int64_t monotonic_clock_ns) {

filament/backend/test/test_Callbacks.cpp

@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2024 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.
+ */
+
+#include "BackendTest.h"
+
+using namespace filament;
+using namespace filament::backend;
+
+namespace test {
+
+TEST_F(BackendTest, FrameScheduledCallback) {
+    auto& api = getDriverApi();
+
+    // Create a SwapChain.
+    // In order for the frameScheduledCallback to be called, this must be a real SwapChain (not
+    // headless) so we obtain a drawable.
+    auto swapChain = createSwapChain();
+
+    Handle<HwRenderTarget> renderTarget = api.createDefaultRenderTarget();
+
+    int callbackCountA = 0;
+    api.setFrameScheduledCallback(swapChain, nullptr, [&callbackCountA](PresentCallable callable) {
+        callable();
+        callbackCountA++;
+    });
+
+    // Render the first frame.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.beginRenderPass(renderTarget, {});
+    api.endRenderPass(0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    // Render the next frame. The same callback should be called.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.beginRenderPass(renderTarget, {});
+    api.endRenderPass(0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    // Now switch out the callback.
+    int callbackCountB = 0;
+    api.setFrameScheduledCallback(swapChain, nullptr, [&callbackCountB](PresentCallable callable) {
+        callable();
+        callbackCountB++;
+    });
+
+    // Render one final frame.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.beginRenderPass(renderTarget, {});
+    api.endRenderPass(0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    api.finish();
+
+    executeCommands();
+    getDriver().purge();
+
+    EXPECT_EQ(callbackCountA, 2);
+    EXPECT_EQ(callbackCountB, 1);
+}
+
+TEST_F(BackendTest, FrameCompletedCallback) {
+    auto& api = getDriverApi();
+
+    // Create a SwapChain.
+    auto swapChain = api.createSwapChainHeadless(256, 256, 0);
+
+    int callbackCountA = 0;
+    api.setFrameCompletedCallback(swapChain, nullptr,
+            [&callbackCountA]() { callbackCountA++; });
+
+    // Render the first frame.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    // Render the next frame. The same callback should be called.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    // Now switch out the callback.
+    int callbackCountB = 0;
+    api.setFrameCompletedCallback(swapChain, nullptr,
+            [&callbackCountB]() { callbackCountB++; });
+
+    // Render one final frame.
+    api.makeCurrent(swapChain, swapChain);
+    api.beginFrame(0, 0, 0);
+    api.commit(swapChain);
+    api.endFrame(0);
+
+    api.finish();
+
+    executeCommands();
+    getDriver().purge();
+
+    EXPECT_EQ(callbackCountA, 2);
+    EXPECT_EQ(callbackCountB, 1);
+}
+
+} // namespace test

filament/src/details/SwapChain.cpp

@@ -79,24 +79,11 @@ bool FSwapChain::isFrameScheduledCallbackSet() const noexcept {
     return mFrameScheduledCallbackIsSet;
 }
 
-void FSwapChain::setFrameCompletedCallback(backend::CallbackHandler* handler,
-                utils::Invocable<void(SwapChain*)>&& callback) noexcept {
-    struct Callback {
-        utils::Invocable<void(SwapChain*)> f;
-        SwapChain* s;
-        static void func(void* user) {
-            auto* const c = reinterpret_cast<Callback*>(user);
-            c->f(c->s);
-            delete c;
-        }
-    };
-    if (callback) {
-        auto* const user = new(std::nothrow) Callback{ std::move(callback), this };
-        mEngine.getDriverApi().setFrameCompletedCallback(
-                mHwSwapChain, handler, &Callback::func, static_cast<void*>(user));
-    } else {
-        mEngine.getDriverApi().setFrameCompletedCallback(mHwSwapChain, nullptr, nullptr, nullptr);
-    }
+void FSwapChain::setFrameCompletedCallback(
+        backend::CallbackHandler* handler, FrameCompletedCallback&& callback) noexcept {
+    using namespace std::placeholders;
+    auto boundCallback = std::bind(std::move(callback), this);
+    mEngine.getDriverApi().setFrameCompletedCallback(mHwSwapChain, handler, std::move(boundCallback));
 }
 
 bool FSwapChain::isSRGBSwapChainSupported(FEngine& engine) noexcept {