Segfault on dynamic data update

[asafran]

Describe the bug
When using multiple command graphs with separate recordAndSubmitTasks _dynamicDataMap in a transfer task can be filled with uninitialized data from another command graph. This happens because the TransferTasks is initialized for each recordAndSubmitTask separately, but the collectResources passes for all recordAndSubmitTasks collecting dynamic buffers, so TransferTasks get dynamic buffers that may not be used in any commandGraphs from associated recordAndSubmitTask. Since initialization occurs sequentially, some buffers from other recordAndSubmitTasks pass in TransferTask may be uninitialized.

Example:
Let's say we have bufferInfo0 and bufferInfo1, both contain dynamic data, are located in separate recordAndSubmitTasks and are not compiled (no VkBuffer assigned).

VulkanSceneGraph/src/vsg/app/Viewer.cpp

Lines 233 to 245 in c169dbe

	using DeviceResourceMap = std::map<ref_ptr<vsg::Device>, DeviceResources>;
	DeviceResourceMap deviceResourceMap;
	for (auto& task : recordAndSubmitTasks)
	{
	auto& collectResources = deviceResourceMap[task->device].collectResources;
	for (auto& commandGraph : task->commandGraphs)
	{
	commandGraph->accept(collectResources);
	}
	if (task->databasePager && !databasePager) databasePager = task->databasePager;
	}

We have all the dynamic bufferInfos in collectResources.

VulkanSceneGraph/src/vsg/app/Viewer.cpp

Lines 300 to 313 in c169dbe

	for (auto& task : recordAndSubmitTasks)
	{
	auto& deviceResource = deviceResourceMap[task->device];
	auto& resourceRequirements = deviceResource.collectResources.requirements;
	bool task_containsPagedLOD = false;
	for (auto& commandGraph : task->commandGraphs)
	{
	commandGraph->maxSlot = resourceRequirements.maxSlot;
	commandGraph->accept(*deviceResource.compile);
	if (resourceRequirements.containsPagedLOD) task_containsPagedLOD = true;
	}

Processing the first recordAndSubmitTask:

• bufferInfo0 compiled, Buffer object created and assigned
• bufferInfo1 not compiled, Buffer object is null
  

  VulkanSceneGraph/src/vsg/app/Viewer.cpp

  Line 327 in c169dbe

  task->earlyTransferTask->assign(resourceRequirements.earlyDynamicData);

  
  Dynamic data info is passed to earlyTranfserTask:
  

  VulkanSceneGraph/src/vsg/app/TransferTask.cpp

  Line 87 in c169dbe

  _dynamicDataMap[bufferInfo->buffer][bufferInfo->offset] = bufferInfo;

_dynamicDataMap from the first recordAndSubmitTask contains as keys:

• compiled Buffer object from bufferInfo0
• null Buffer object from bufferInfo1

We should collect and compile dynamic buffers for each task separately to avoid null Buffer objects in _dynamicDataMap.

To Reproduce
Assign the viewer several recordAndSubmitTasks, each of which contains dynamic data in.

Desktop (please complete the following information):

• OS: Fedora 37
• VSG-v1.0.4

[robertosfield]

Do have a code that I could reproduce the bug with?

[robertosfield]

I am currently investigating this issue, and have modified the vsgwindows example with assumption this would be a good first start at creating a test bed for this problem usage case,

I would really appreciate more info on how to set up things up with your usage case to avoid any possibility of me implementing something that doesn't produce the problem and/or doesn't recreate the actual problem you have. As this is threading issue it can be really awkward to reproduce issues and just as hard to know for sure the issue as has been fully resolved.

[robertosfield]

I have modified the vsgwindows example to have a dynamic vertex update code path that is enabled using --dynamic:, and enabling of multi-threading using --mt, usage including enabling dynamic is:

vsgwindows models/lz.vsgt models/teapot.vsgt --dynamic -d --mt

The branch of vsgExamples with the changes is:

https://github.com/vsg-dev/vsgExamples/tree/DynamicMultiWindows

I don't get any crashes yet. I have made some fixes to vsg::Trackball to handle the multi window usage, and to vsg::Array::create(..) to handle the array duplication, so you'll need VSG master to use the above vsgExample branch.

I have run out of working day to investigate further so will have to return to this tomorrow.

[asafran]

vsgdynamictexture_cs.zip
Here is a modified example of vsgdynamictexture_cs to reproduce this bug

[robertosfield]

Thanks. I will have a look tomorrow when I am back at my Dev system. Should I expect a seg fault or other symptoms? Does the bug appear right away?

…

On Thu, 6 Apr 2023, 19:06 Andrei Safranovich, ***@***.***> wrote: vsgdynamictexture_cs.zip <https://github.com/vsg-dev/VulkanSceneGraph/files/11173170/vsgdynamictexture_cs.zip> Here is a modified example of vsgdynamictexture_cs to reproduce this bug — Reply to this email directly, view it on GitHub <#764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKEGUHLN3BUFWKLOGFFQODW74AYTANCNFSM6AAAAAAWUTCRZ4> . You are receiving this because you commented.Message ID: ***@***.***>

[robertosfield]

I have built your modified example and get the crash:

sg::Buffer::vk (this=0x0, deviceID=0) at /home/robert/Dev/VulkanSceneGraph/include/vsg/state/Buffer.h:31
31 VkBuffer vk(uint32_t deviceID) const { return _vulkanData[deviceID].buffer; }
(gdb) where
#0 vsg::Buffer::vk (this=0x0, deviceID=0) at /home/robert/Dev/VulkanSceneGraph/include/vsg/state/Buffer.h:31
#1 0x000055555570f207 in vsg::TransferTask::_transferBufferInfos (this=0x7ffff767ddc8, vk_commandBuffer=0x555557b61050, frame=...,
offset=@0x7fffffffc910: 4) at /home/robert/Dev/VulkanSceneGraph/src/vsg/app/TransferTask.cpp:163
#2 0x000055555571131b in vsg::TransferTask::transferDynamicData (this=0x7ffff767ddc8)
at /home/robert/Dev/VulkanSceneGraph/src/vsg/app/TransferTask.cpp:639
#3 0x00005555557092d1 in vsg::RecordAndSubmitTask::submit (this=0x7ffff767dbe8, frameStamp=...)
at /home/robert/Dev/VulkanSceneGraph/src/vsg/app/RecordAndSubmitTask.cpp:85
#4 0x00005555556cb229 in vsg::Viewer::recordAndSubmit (this=0x7ffff767c5c0)
at /home/robert/Dev/VulkanSceneGraph/src/vsg/app/Viewer.cpp:691
#5 0x00005555556239e1 in main (argc=1, argv=0x7fffffffd298)
at /home/robert/Dev/vsgExamples/examples/state/vsgdynamictexture_cs/vsgdynamictexture_cs.cpp:342

This gives me something concrete to go after now as my attempt yesterday to get vsgwindows to run multi-threaded with dynamic vertex data didn't provoke any problem.

After my code review yesterday I was reminded that I hadn't complete the changes I had in mind when I implemented TrasnferTask. My long term plan has been for TransferTask to sit alongside RecordAndSubmitTask so that there is one TransferTask per Device rather than one per RecordAndSubmitTask. The change may well address the issue above.

[robertosfield]

To resolve the problem seen with the modified vsgdynanmictexture_cs by moving the BufferInfo assignment to the TransferTasks to after all the RecordAndSubmitTasks have been compiled. This fix is now merged with #771.

I haven't attempted the change to have one TransferTask per Device, I'll leave that to a future date as this is a bit more involved than just a bug fix. I'll close this Issue as I believe the problem is solved for this particular usage case.

If problems still persist then this will be a different bug than the one I've just resolved and we'll need to create a new test example to reproduce it, so opening a new issue would be then appropriate.