fdo-mirrors/mesa
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/mesa/mesa.git synced 2026-05-07 04:58:05 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

screenshot-layer: Fix race on writing the .pngs vs device destroy.

Browse source 
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: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/41299>
This commit is contained in:
Emma Anholt 2026-04-23 15:05:01 -07:00 • committed by Marge Bot
parent 3512886aa7
commit 25b2865146
1 changed files with 40 additions and 37 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
src/vulkan/screenshot-layer/screenshot.cpp
View file

@ -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);
}