Better handling of engine shutdown

- we make sure we don't leak user callbacks or fences
- fix typos

android/filament-android/src/main/java/com/google/android/filament/Texture.java

@@ -322,13 +322,26 @@ public class Texture {
         public CompressedFormat compressedFormat;
 
         @Nullable public Object handler;
-        @Nullable public Runnable callback;
 
         /**
-         * Valid handler types:
-         * - Android: Handler, Executor
-         * - Other: Executor
+         * Callback used to destroy the buffer data.
+         * <p>
+         * Guarantees:
+         * <ul>
+         *     <li>Called on the main filament thread.</li>
+         * </ul>
+         * </p>
+         *
+         * <p>
+         * Limitations:
+         * <ul>
+         *     <li>Must be lightweight.</li>
+         *     <li>Must not call filament APIs.</li>
+         * </ul>
+         * </p>
          */
+        @Nullable public Runnable callback;
+
 
         /**
          * Creates a <code>PixelBufferDescriptor</code>

filament/backend/include/backend/BufferDescriptor.h

@@ -39,7 +39,12 @@ class UTILS_PUBLIC BufferDescriptor {
 public:
     /**
      * Callback used to destroy the buffer data.
-     * It is guaranteed to be called on the main filament thread.
+     * Guarantees:
+     *      Called on the main filament thread.
+     *
+     * Limitations:
+     *      Must be lightweight.
+     *      Must not call filament APIs.
      */
     using Callback = void(*)(void* buffer, size_t size, void* user);
 

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -163,6 +163,15 @@ OpenGLDriver::~OpenGLDriver() noexcept {
 // ------------------------------------------------------------------------------------------------
 
 void OpenGLDriver::terminate() {
+    // wait for the GPU to finish executing all commands
+    glFinish();
+
+    // and make sure to execute all the GpuCommandCompleteOps callbacks
+    executeGpuCommandsCompleteOps();
+
+    // because we called glFinish(), all callbacks should have been executed
+    assert(!mGpuCommandCompleteOps.size());
+
     for (auto& item : mSamplerMap) {
         mContext.unbindSampler(item.second);
         glDeleteSamplers(1, &item.second);
@@ -2428,7 +2437,7 @@ void OpenGLDriver::readPixels(Handle<HwRenderTarget> src,
     glReadPixels(GLint(x), GLint(y), GLint(width), GLint(height), glFormat, glType, nullptr);
     gl.bindBuffer(GL_PIXEL_PACK_BUFFER, 0);
 
-    // we're forced to make a copy on the stack because otherwise it deletes std::function<> copy
+    // we're forced to make a copy on the heap because otherwise it deletes std::function<> copy
     // constructor.
     auto* pUserBuffer = new PixelBufferDescriptor(std::move(p));
     whenGpuCommandsComplete([this, width, height, pbo, pUserBuffer]() mutable {
@@ -2443,7 +2452,7 @@ void OpenGLDriver::readPixels(Handle<HwRenderTarget> src,
                     p.format, p.type, 1, 1, 1);
             size_t bpr = PixelBufferDescriptor::computeDataSize(
                     p.format, p.type, stride, 1, p.alignment);
-            char* head = (char*)vaddr + p.left * bpp + bpr * p.top;
+            char const* head = (char const*)vaddr + p.left * bpp + bpr * p.top;
             char* tail = (char*)p.buffer + p.left * bpp + bpr * (p.top + height - 1);
             for (size_t i = 0; i < height; ++i) {
                 memcpy(tail, head, bpp * width);
@@ -2534,6 +2543,7 @@ void OpenGLDriver::flush(int) {
 void OpenGLDriver::finish(int) {
     DEBUG_MARKER()
     glFinish();
+    executeGpuCommandsCompleteOps();
 }
 
 UTILS_NOINLINE

filament/backend/src/opengl/OpenGLDriver.h

@@ -379,7 +379,7 @@ private:
 
     void whenGpuCommandsComplete(std::function<void()> fn) noexcept;
     void executeGpuCommandsCompleteOps() noexcept;
-    std::vector< std::pair<GLsync, std::function<void(void)>> > mGpuCommandCompleteOps;
+    std::vector<std::pair<GLsync, std::function<void(void)>>> mGpuCommandCompleteOps;
 };
 
 // ------------------------------------------------------------------------------------------------

filament/src/Engine.cpp

@@ -315,21 +315,33 @@ void FEngine::shutdown() {
     }
     cleanupResourceList(mFences);
 
-    // There might be commands added by the terminate() calls
-    flushCommandBuffer(mCommandBufferQueue);
-    if (!UTILS_HAS_THREADING) {
-        execute();
-    }
-
     /*
-     * terminate the rendering engine
+     * Shutdown the backend...
      */
 
+    // There might be commands added by the terminate() calls, so we need to flush all commands
+    // up to this point. After flushCommandBuffer() is called, all pending commands are guaranteed
+    // to be executed before the driver thread exits.
+    flushCommandBuffer(mCommandBufferQueue);
+
+    // now wait for all pending commands to be executed and the thread to exit
     mCommandBufferQueue.requestExit();
-    if (UTILS_HAS_THREADING) {
+    if (!UTILS_HAS_THREADING) {
+        execute();
+        getDriverApi().terminate();
+    } else {
         mDriverThread.join();
+
     }
 
+    // Finally, call user callbacks that might have been scheduled.
+    // These callbacks CANNOT call driver APIs.
+    getDriver().purge();
+
+    /*
+     * Terminate the JobSystem...
+     */
+
     // detach this thread from the jobsystem
     mJobSystem.emancipate();
 
@@ -446,6 +458,9 @@ int FEngine::loop() {
     }
 #endif
 
+    JobSystem::setThreadName("FEngine::loop");
+    JobSystem::setThreadPriority(JobSystem::Priority::DISPLAY);
+
     mDriver = platform->createDriver(mSharedGLContext);
     mDriverBarrier.latch();
     if (UTILS_UNLIKELY(!mDriver)) {
@@ -454,9 +469,6 @@ int FEngine::loop() {
         return 0;
     }
 
-    JobSystem::setThreadName("FEngine::loop");
-    JobSystem::setThreadPriority(JobSystem::Priority::DISPLAY);
-
     // We use the highest affinity bit, assuming this is a Big core in a  big.little
     // configuration. This is also a core not used by the JobSystem.
     // Either way the main reason to do this is to avoid this thread jumping from core to core