From 25b2865146aa4578e8322b4f034e7735b8a5cee3 Mon Sep 17 00:00:00 2001 From: Emma Anholt Date: Thu, 23 Apr 2026 15:05:01 -0700 Subject: [PATCH] screenshot-layer: Fix race on writing the .pngs vs device destroy. The writePNG thread function accessed the VkDevice, but the layer had no joining of the thread on device destroy. Instead of rolling our own job queue for the writePNG job, reuse u_queue, which handles all the hard stuff for us. Assisted-by: Claude Opus 4.6 Part-of: --- src/vulkan/screenshot-layer/screenshot.cpp | 77 +++++++++++----------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/src/vulkan/screenshot-layer/screenshot.cpp b/src/vulkan/screenshot-layer/screenshot.cpp index a3f6b7c78c4..f681a969d9f 100644 --- a/src/vulkan/screenshot-layer/screenshot.cpp +++ b/src/vulkan/screenshot-layer/screenshot.cpp @@ -65,6 +65,7 @@ #include "util/os_socket.h" #include "util/simple_mtx.h" #include "util/u_math.h" +#include "util/u_queue.h" #include "vk_enum_to_str.h" #include "vk_dispatch_table.h" @@ -103,9 +104,6 @@ struct instance_data { const char *filename; }; -pthread_cond_t ptCondition = PTHREAD_COND_INITIALIZER; -pthread_mutex_t ptLock = PTHREAD_MUTEX_INITIALIZER; - const VkPipelineStageFlags dstStageWaitBeforeSubmission = VK_PIPELINE_STAGE_BOTTOM_OF_PIPE_BIT; const VkSemaphore *pSemaphoreWaitBeforePresent; uint32_t semaphoreWaitBeforePresentCount; @@ -127,6 +125,7 @@ struct device_data { struct queue_data *graphic_queue; struct queue_data* queue_data_head; struct queue_data* queue_data_tail; + struct util_queue png_write_queue; }; /* Mapped from VkQueue */ @@ -299,6 +298,8 @@ static struct device_data *new_device_data(VkDevice device, struct instance_data data->graphic_queue = VK_NULL_HANDLE; data->queue_data_head = VK_NULL_HANDLE; data->queue_data_tail = VK_NULL_HANDLE; + util_queue_init(&data->png_write_queue, "screenshot_png_write", 4, 1, + UTIL_QUEUE_INIT_RESIZE_IF_FULL, data); map_object(HKEY(data->device), data); return data; } @@ -720,6 +721,7 @@ VkQueue getQueueForScreenshot(struct device_data *device_data, // and clean them up when they go out of scope. struct WriteFileCleanupData { device_data *dev_data; + VkFence fence; VkImage image2; VkImage image3; VkDeviceMemory mem2; @@ -728,7 +730,6 @@ struct WriteFileCleanupData { bool mem3mapped; VkCommandBuffer commandBuffer; VkCommandPool commandPool; - VkFence fence; ~WriteFileCleanupData(); }; @@ -768,24 +769,22 @@ static void print_time_difference(long int start_time, long int end_time) { // Store all data required for threading the saving to file functionality struct ThreadSaveData { - struct device_data *device_data; const char *filename; const char *pFramebuffer; VkSubresourceLayout srLayout; - VkFence fence; - uint32_t const width; - uint32_t const height; - uint32_t const numChannels; + uint32_t width; + uint32_t height; + uint32_t numChannels; + WriteFileCleanupData cleanup; }; /* Write the copied image to a PNG file */ -void *writePNG(void *data) { - struct ThreadSaveData *threadData = (struct ThreadSaveData*)data; +void writePNG(void *job, void *gdata, UNUSED int thread_index) { + struct ThreadSaveData *threadData = (struct ThreadSaveData*)job; + struct device_data *device_data = (struct device_data*)gdata; FILE *file; - size_t length = sizeof(char[LARGE_BUFFER_SIZE+STANDARD_BUFFER_SIZE]); const char *tmpStr = ".tmp"; - char *filename = (char *)malloc(length); - char *tmpFilename = (char *)malloc(length + 4); // Allow for ".tmp" + char *tmpFilename = (char *)malloc(strlen(threadData->filename) + 1 + 4); // Allow for ".tmp" png_byte *row_pointer; png_infop info = NULL; png_struct* png; @@ -797,9 +796,7 @@ void *writePNG(void *data) { int localWidth = threadData->width; int numChannels = threadData->numChannels; int matrixSize = localHeight * rowPitch; - bool checks_failed = true; - memcpy(filename, threadData->filename, length); - memcpy(tmpFilename, threadData->filename, length); + strcpy(tmpFilename, threadData->filename); strcat(tmpFilename, tmpStr); file = fopen(tmpFilename, "wb"); //create file for output if (!file) { @@ -821,9 +818,12 @@ void *writePNG(void *data) { LOG(ERROR, "setjmp() failed\n"); goto cleanup; } - threadData->device_data->vtable.WaitForFences(threadData->device_data->device, 1, &threadData->fence, VK_TRUE, UINT64_MAX); + device_data->vtable.WaitForFences(device_data->device, 1, &threadData->cleanup.fence, VK_TRUE, UINT64_MAX); threadData->pFramebuffer += threadData->srLayout.offset; start_time = get_time(); + + // Copy from the image data to a temporary, in case we have a non-cached + // buffer that we wouldn't want to be png encoding from. row_pointer = (png_byte *)malloc(sizeof(png_byte) * matrixSize); memcpy(row_pointer, threadData->pFramebuffer, matrixSize); /* Ensure alpha bits are set to 'opaque' if image is of RGBA format */ @@ -834,9 +834,6 @@ void *writePNG(void *data) { } end_time = get_time(); print_time_difference(start_time, end_time); - // We've created all local copies of data, - // so let's signal main thread to continue - pthread_cond_signal(&ptCondition); png_init_io(png, file); // Initialize file output png_set_IHDR( // Set image properties png, // Pointer to png_struct @@ -868,21 +865,19 @@ void *writePNG(void *data) { // Rename file, indicating completion, client should be // checking for the final file exists. - if (rename(tmpFilename, filename) != 0 ) - LOG(ERROR, "Could not rename from '%s' to '%s'\n", tmpFilename, filename); + if (rename(tmpFilename, threadData->filename) != 0 ) + LOG(ERROR, "Could not rename from '%s' to '%s'\n", tmpFilename, threadData->filename); else - LOG(INFO, "Successfully renamed from '%s' to '%s'\n", tmpFilename, filename); - checks_failed = false; + LOG(INFO, "Successfully renamed from '%s' to '%s'\n", tmpFilename, threadData->filename); cleanup: - if (checks_failed) - pthread_cond_signal(&ptCondition); if (info) png_destroy_write_struct(&png, &info); if (file) fclose(file); - free(filename); + threadData->cleanup.~WriteFileCleanupData(); + free((void *)threadData->filename); + free(threadData); free(tmpFilename); - return nullptr; } /* Write an image to file. Upon encountering issues, do not impact the @@ -1223,16 +1218,21 @@ static bool write_image( data.mem3mapped = true; } - // Thread off I/O operations - pthread_t ioThread; - pthread_mutex_lock(&ptLock); // Grab lock, we need to wait until thread has copied values of pointers - struct ThreadSaveData threadData = {device_data, filename, pFramebuffer, srLayout, data.fence, newWidth, newHeight, numChannels}; + struct ThreadSaveData *threadData = (struct ThreadSaveData *)calloc(1, sizeof(*threadData)); + threadData->filename = strdup(filename); + threadData->pFramebuffer = pFramebuffer; + threadData->srLayout = srLayout; + threadData->width = newWidth; + threadData->height = newHeight; + threadData->numChannels = numChannels; + // Transfer ownership of VK resources to the thread so they remain valid + // until writePNG is done with them. Zero out data so its destructor is + // a no-op when write_image() returns. + threadData->cleanup = data; + data = WriteFileCleanupData {}; // Write the data to a PNG file. - pthread_create(&ioThread, NULL, writePNG, (void *)&threadData); - pthread_detach(ioThread); // Reclaim resources once thread terminates - pthread_cond_wait(&ptCondition, &ptLock); - pthread_mutex_unlock(&ptLock); + util_queue_add_job(&device_data->png_write_queue, threadData, NULL, writePNG, NULL, 0); return true; } @@ -1444,6 +1444,9 @@ static void screenshot_DestroyDevice( const VkAllocationCallbacks* pAllocator) { struct device_data *device_data = FIND(struct device_data, device); + + util_queue_destroy(&device_data->png_write_queue); + device_data->vtable.DestroyDevice(device, pAllocator); destroy_device_data(device_data); }