Repository navigation
layersvt: Remove DebugMarker object names on null or empty name - #47
olehkuznetsov wants to merge 1 commit into
Conversation
| } | ||
|
|
||
| void DebugMarker::SetDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const char* name) { | ||
| // VK_EXT_debug_utils defines a null or empty name as removing the object's name. |
There was a problem hiding this comment.
vkDebugMarkerSetObjectNameEXT also routes through here. VK_EXT_debug_marker requires a non-null pObjectName and does not define empty-string semantics, so treating empty as removal there is a choice (a reasonable one). Maybe say so, e.g. "VK_EXT_debug_utils defines ...; applied to VK_EXT_debug_marker for consistency."
| void DebugMarker::RemoveDebugObjectName(uint64_t device, int32_t type, uint64_t handle) { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
| if (debug_object_names_.erase(std::make_tuple(device, type, handle)) == 0) return; | ||
| // The empty name tells trace consumers that the object is unnamed from this timestamp on. |
There was a problem hiding this comment.
Which consumer relies on the empty-name convention? object_name is a proto2 optional field, so leaving it unset would also be an option and is distinguishable from an explicit empty string. Either works, but it would be good to document the convention next to EmitDebugObjectNamePacket (or in the proto) so consumers and future emitters agree on it.
| void SetDebugObjectName(uint64_t device, int32_t type, uint64_t handle, const char* name); | ||
|
|
||
| /** | ||
| * Removes the name of an object and, if it was named, emits an empty name to the trace so that |
There was a problem hiding this comment.
This motivates the method by handle reuse after destruction, which isn't wired up in this change; today's caller is name clearing. Consider leading with that, and matching the @brief/@PARAM style used by the neighbouring declarations.
| DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, "Image"); | ||
| DebugMarker::Get().SetDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, ""); | ||
| EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, "Image")); | ||
| EXPECT_FALSE(DebugMarker::Get().HasDebugObjectName(device, VK_OBJECT_TYPE_IMAGE, 0x2, "")); |
There was a problem hiding this comment.
Nit: HasDebugObjectName returns false both when the entry is gone and when it holds a different name, so these checks can't fully prove the entry was erased. The "Buffer"/"NULL" checks cover the regression in practice; a test-only presence check would make it strict if you care. The empty-name packet emission is also untested, which seems fine given there's no trace-capture fixture yet.
74e9e95 to
112f8a8
Compare
VK_EXT_debug_utils defines a null or empty pObjectName as removing the object's name. DebugMarker stored and emitted the literal "NULL" instead, so cleared objects appeared as "NULL" in traces and were replayed under that name on every later session start. Erase the stored name instead and emit an empty name, so trace consumers treat the object as unnamed from that point on. The removal is a separate method so object destruction can reuse it. Bug: Test: new tests - DebugMarkerTests#NullOrEmptyNameRemovesNameTest Change-Id: Ia6d7df2386b911a0b50911ebc7c5e9176a6a6964
112f8a8 to
c7cfb9f
Compare
2 participants
VK_EXT_debug_utils defines a null or empty pObjectName as removing the
object's name. DebugMarker stored and emitted the literal "NULL"
instead, so cleared objects appeared as "NULL" in traces and were
replayed under that name on every later session start.
Erase the stored name instead and emit an empty name, so trace
consumers treat the object as unnamed from that point on. The removal
is a separate method so object destruction can reuse it.
Bug:
Test: new tests - DebugMarkerTests#NullOrEmptyNameRemovesNameTest
Change-Id: Ia6d7df2386b911a0b50911ebc7c5e9176a6a6964