Skip to content

Commit

Permalink
Vulkan: Fix a crash caused by freeing Effects too early
Browse files Browse the repository at this point in the history
  • Loading branch information
@flibitijibibo
flibitijibibo committed Jan 18, 2024
1 parent e74fe86 commit af6fd47
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 79 deletions.
106 changes: 58 additions & 48 deletions src/FNA3D_CommandBuffer.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ typedef struct FNA3D_CommandBufferContainer
uint32_t boundBufferCount;
uint32_t boundBufferCapacity;

FNA3D_Effect **boundEffects;
uint32_t boundEffectCount;
uint32_t boundEffectCapacity;

FNA3D_Renderbuffer **renderbuffersToDestroy;
uint32_t renderbuffersToDestroyCount;
uint32_t renderbuffersToDestroyCapacity;
Expand All @@ -64,10 +68,6 @@ typedef struct FNA3D_CommandBufferContainer
uint32_t buffersToDestroyCount;
uint32_t buffersToDestroyCapacity;

FNA3D_Effect **effectsToDestroy;
uint32_t effectsToDestroyCount;
uint32_t effectsToDestroyCapacity;

FNA3D_Texture **texturesToDestroy;
uint32_t texturesToDestroyCount;
uint32_t texturesToDestroyCapacity;
Expand Down Expand Up @@ -116,14 +116,20 @@ static FNA3D_CommandBufferContainer* FNA3D_INTERNAL_CreateCommandBufferContainer
result->transferBufferCount = 0;
result->transferBuffers = NULL;

/* Bound buffer tracking */
/* Bound resources tracking */

result->boundBufferCapacity = 4;
result->boundBufferCount = 0;
result->boundBuffers = (FNA3D_BufferHandle**) SDL_malloc(
result->boundBufferCapacity * sizeof(FNA3D_BufferHandle*)
);

result->boundEffectCapacity = 4;
result->boundEffectCount = 0;
result->boundEffects = (FNA3D_Effect**) SDL_malloc(
result->boundEffectCapacity * sizeof(FNA3D_Effect*)
);

/* Destroyed resources tracking */

result->renderbuffersToDestroyCapacity = 16;
Expand All @@ -142,14 +148,6 @@ static FNA3D_CommandBufferContainer* FNA3D_INTERNAL_CreateCommandBufferContainer
result->buffersToDestroyCapacity
);

result->effectsToDestroyCapacity = 16;
result->effectsToDestroyCount = 0;

result->effectsToDestroy = (FNA3D_Effect**) SDL_malloc(
sizeof(FNA3D_Effect*) *
result->effectsToDestroyCapacity
);

result->texturesToDestroyCapacity = 16;
result->texturesToDestroyCount = 0;

Expand Down Expand Up @@ -181,6 +179,19 @@ static void FNA3D_CommandBuffer_CleanContainer(
}
container->boundBufferCount = 0;

/* Reset bound effects */
for (i = 0; i < container->boundEffectCount; i += 1)
{
if (container->boundEffects[i] != NULL)
{
manager->driver.DecEffectRef(
manager->driver.driverData,
container->boundEffects[i]
);
}
}
container->boundEffectCount = 0;

/* Destroy resources marked for destruction */

for (i = 0; i < container->renderbuffersToDestroyCount; i += 1)
Expand All @@ -201,15 +212,6 @@ static void FNA3D_CommandBuffer_CleanContainer(
}
container->buffersToDestroyCount = 0;

for (i = 0; i < container->effectsToDestroyCount; i += 1)
{
manager->driver.DestroyEffect(
manager->driver.driverData,
container->effectsToDestroy[i]
);
}
container->effectsToDestroyCount = 0;

for (i = 0; i < container->texturesToDestroyCount; i += 1)
{
manager->driver.DestroyTexture(
Expand Down Expand Up @@ -373,10 +375,10 @@ void FNA3D_DestroyCommandBufferManager(

SDL_free(commandBufferContainer->transferBuffers);
SDL_free(commandBufferContainer->boundBuffers);
SDL_free(commandBufferContainer->boundEffects);

SDL_free(commandBufferContainer->renderbuffersToDestroy);
SDL_free(commandBufferContainer->buffersToDestroy);
SDL_free(commandBufferContainer->effectsToDestroy);
SDL_free(commandBufferContainer->texturesToDestroy);

SDL_free(commandBufferContainer);
Expand Down Expand Up @@ -577,31 +579,6 @@ void FNA3D_CommandBuffer_AddDisposeRenderbuffer(
SDL_UnlockMutex(manager->commandLock);
}

void FNA3D_CommandBuffer_AddDisposeEffect(
FNA3D_CommandBufferManager *manager,
FNA3D_Effect *effect
) {
FNA3D_CommandBufferContainer *container;

SDL_LockMutex(manager->commandLock);

container = manager->currentCommandBufferContainer;

if (container->effectsToDestroyCount + 1 >= container->effectsToDestroyCapacity)
{
container->effectsToDestroyCapacity *= 2;

container->effectsToDestroy = SDL_realloc(
container->effectsToDestroy,
sizeof(FNA3D_Effect*) * container->effectsToDestroyCapacity
);
}
container->effectsToDestroy[container->effectsToDestroyCount] = effect;
container->effectsToDestroyCount += 1;

SDL_UnlockMutex(manager->commandLock);
}

void FNA3D_CommandBuffer_AddDisposeBuffers(
FNA3D_CommandBufferManager *manager,
FNA3D_BufferHandle **handles,
Expand Down Expand Up @@ -717,6 +694,39 @@ void FNA3D_CommandBuffer_MarkBufferAsBound(
manager->currentCommandBufferContainer->boundBufferCount += 1;
}

void FNA3D_CommandBuffer_MarkEffectAsBound(
FNA3D_CommandBufferManager *manager,
FNA3D_Effect *effect
) {
uint32_t i;

for (i = 0; i < manager->currentCommandBufferContainer->boundEffectCount; i += 1)
{
if (effect == manager->currentCommandBufferContainer->boundEffects[i])
{
/* Effect is already referenced, nothing to do */
return;
}
}

manager->driver.IncEffectRef(
manager->driver.driverData,
effect
);

if (manager->currentCommandBufferContainer->boundEffectCount >= manager->currentCommandBufferContainer->boundEffectCapacity)
{
manager->currentCommandBufferContainer->boundEffectCapacity *= 2;
manager->currentCommandBufferContainer->boundEffects = SDL_realloc(
manager->currentCommandBufferContainer->boundEffects,
manager->currentCommandBufferContainer->boundEffectCapacity * sizeof(FNA3D_BufferHandle*)
);
}

manager->currentCommandBufferContainer->boundEffects[manager->currentCommandBufferContainer->boundEffectCount] = effect;
manager->currentCommandBufferContainer->boundEffectCount += 1;
}

FNA3D_TransferBuffer* FNA3D_CommandBuffer_AcquireTransferBuffer(
FNA3D_CommandBufferManager *manager,
size_t requiredSize,
Expand Down
26 changes: 16 additions & 10 deletions src/FNA3D_CommandBuffer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,15 @@ typedef struct FNA3D_CommandBufferDriver
FNA3D_BufferHandle *handle
);

void (*IncEffectRef)(
FNA3D_Renderer *renderer,
FNA3D_Effect *handle
);
void (*DecEffectRef)(
FNA3D_Renderer *renderer,
FNA3D_Effect *handle
);

void (*DestroyTexture)(
FNA3D_Renderer *driverData,
FNA3D_Texture *texture
Expand All @@ -103,10 +112,6 @@ typedef struct FNA3D_CommandBufferDriver
FNA3D_Renderer *driverData,
FNA3D_Renderbuffer *renderbuffer
);
void (*DestroyEffect)(
FNA3D_Renderer *driverData,
FNA3D_Effect *effect
);

FNA3D_Renderer *driverData;
} FNA3D_CommandBufferDriver;
Expand All @@ -126,10 +131,11 @@ typedef struct FNA3D_CommandBufferDriver
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(IncBufferRef, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DecBufferRef, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(GetBufferSize, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(IncEffectRef, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DecEffectRef, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DestroyTexture, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DestroyBuffer, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DestroyRenderbuffer, name) \
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DestroyEffect, name)
ASSIGN_COMMANDBUFFER_DRIVER_FUNC(DestroyRenderbuffer, name)

/*
* Command Buffer Interface
Expand Down Expand Up @@ -175,10 +181,6 @@ FNA3D_SHAREDINTERNAL void FNA3D_CommandBuffer_AddDisposeRenderbuffer(
FNA3D_CommandBufferManager *manager,
FNA3D_Renderbuffer *renderbuffer
);
FNA3D_SHAREDINTERNAL void FNA3D_CommandBuffer_AddDisposeEffect(
FNA3D_CommandBufferManager *manager,
FNA3D_Effect *effect
);
FNA3D_SHAREDINTERNAL void FNA3D_CommandBuffer_AddDisposeBuffers(
FNA3D_CommandBufferManager *manager,
FNA3D_BufferHandle **handles,
Expand All @@ -195,6 +197,10 @@ FNA3D_SHAREDINTERNAL void FNA3D_CommandBuffer_MarkBufferAsBound(
FNA3D_CommandBufferManager *manager,
FNA3D_BufferHandle *buffer
);
FNA3D_SHAREDINTERNAL void FNA3D_CommandBuffer_MarkEffectAsBound(
FNA3D_CommandBufferManager *manager,
FNA3D_Effect *effect
);
FNA3D_SHAREDINTERNAL FNA3D_TransferBuffer* FNA3D_CommandBuffer_AcquireTransferBuffer(
FNA3D_CommandBufferManager *manager,
size_t requiredSize,
Expand Down
65 changes: 44 additions & 21 deletions src/FNA3D_Driver_Vulkan.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,7 @@ typedef struct VulkanRenderbuffer /* Cast from FNA3D_Renderbuffer* */
typedef struct VulkanEffect /* Cast from FNA3D_Effect* */
{
MOJOSHADER_effect *effect;
SDL_atomic_t refcount;
} VulkanEffect;

typedef struct VulkanQuery /* Cast from FNA3D_Query* */
Expand Down Expand Up @@ -1241,6 +1242,7 @@ typedef struct VulkanRenderer
/* MojoShader Interop */
MOJOSHADER_vkContext *mojoshaderContext;
MOJOSHADER_effect *currentEffect;
FNA3D_Effect *currentFNAEffect;
const MOJOSHADER_effectTechnique *currentTechnique;
uint32_t currentPass;

Expand Down Expand Up @@ -6208,6 +6210,11 @@ static void VULKAN_INTERNAL_BindPipeline(VulkanRenderer *renderer)
VkShaderModule vertShader, fragShader;
MOJOSHADER_vkGetShaderModules(renderer->mojoshaderContext, &vertShader, &fragShader);

FNA3D_CommandBuffer_MarkEffectAsBound(
renderer->commandBuffers,
renderer->currentFNAEffect
);

if ( renderer->needNewPipeline ||
renderer->currentVertShader != vertShader ||
renderer->currentFragShader != fragShader )
Expand Down Expand Up @@ -9693,6 +9700,7 @@ static void VULKAN_CreateEffect(

result = (VulkanEffect*) SDL_malloc(sizeof(VulkanEffect));
result->effect = *effectData;
SDL_AtomicSet(&result->refcount, 1);
*effect = (FNA3D_Effect*) result;
}

Expand All @@ -9714,6 +9722,7 @@ static void VULKAN_CloneEffect(

result = (VulkanEffect*) SDL_malloc(sizeof(VulkanEffect));
result->effect = *effectData;
SDL_AtomicSet(&result->refcount, 1);
*effect = (FNA3D_Effect*) result;
}

Expand All @@ -9722,8 +9731,26 @@ static void VULKAN_AddDisposeEffect(
FNA3D_Effect *effect
) {
VulkanRenderer *renderer = (VulkanRenderer*) driverData;
VulkanEffect *vulkanEffect = (VulkanEffect*) effect;
MOJOSHADER_effect *effectData;

if (SDL_AtomicDecRef(&vulkanEffect->refcount))
{
effectData = vulkanEffect->effect;

if (effectData == renderer->currentEffect)
{
MOJOSHADER_effectEndPass(renderer->currentEffect);
MOJOSHADER_effectEnd(renderer->currentEffect);
renderer->currentFNAEffect = NULL;
renderer->currentEffect = NULL;
renderer->currentTechnique = NULL;
renderer->currentPass = 0;
}

FNA3D_CommandBuffer_AddDisposeEffect(renderer->commandBuffers, effect);
MOJOSHADER_deleteEffect(effectData);
SDL_free(vulkanEffect);
}
}

static void VULKAN_SetEffectTechnique(
Expand Down Expand Up @@ -9791,6 +9818,7 @@ static void VULKAN_ApplyEffect(
);

MOJOSHADER_effectBeginPass(effectData, pass);
renderer->currentFNAEffect = effect;
renderer->currentEffect = effectData;
renderer->currentTechnique = technique;
renderer->currentPass = pass;
Expand Down Expand Up @@ -10923,6 +10951,21 @@ static size_t VULKAN_CommandBuffer_GetBufferSize(
return vulkanBuffer->size;
}

static void VULKAN_CommandBuffer_IncEffectRef(
FNA3D_Renderer *driverData,
FNA3D_Effect *handle
) {
VulkanEffect *vulkanEffect = (VulkanEffect*) handle;
SDL_AtomicIncRef(&vulkanEffect->refcount);
}

static void VULKAN_CommandBuffer_DecEffectRef(
FNA3D_Renderer *driverData,
FNA3D_Effect *handle
) {
VULKAN_AddDisposeEffect(driverData, handle);
}

static void VULKAN_CommandBuffer_DestroyTexture(
FNA3D_Renderer *driverData,
FNA3D_Texture *texture
Expand Down Expand Up @@ -10978,26 +11021,6 @@ static void VULKAN_CommandBuffer_DestroyRenderbuffer(
SDL_free(vkRenderbuffer);
}

static void VULKAN_CommandBuffer_DestroyEffect(
FNA3D_Renderer *driverData,
FNA3D_Effect *effect
) {
VulkanRenderer *renderer = (VulkanRenderer*) driverData;
VulkanEffect *vulkanEffect = (VulkanEffect*) effect;
MOJOSHADER_effect *effectData = vulkanEffect->effect;

if (effectData == renderer->currentEffect)
{
MOJOSHADER_effectEndPass(renderer->currentEffect);
MOJOSHADER_effectEnd(renderer->currentEffect);
renderer->currentEffect = NULL;
renderer->currentTechnique = NULL;
renderer->currentPass = 0;
}
MOJOSHADER_deleteEffect(effectData);
SDL_free(vulkanEffect);
}

/* Driver */

static uint8_t VULKAN_PrepareWindowAttributes(uint32_t *flags)
Expand Down

0 comments on commit af6fd47

Please sign in to comment.