KhronosGroup / KhronosGroup/Vulkan-Samples

timestamp example two problems

Open
#1,368 4 comments 0 reactions 1 assignee Claimed by @SaschaWillems View on GitHub
Dominant language
C++
Stars
5.4k
Forks
835
Avg merge
2h 16m
Merged PRs (30d)
1

Description

I see two issues with [timestamp example](https://docs.vulkan.org/samples/latest/samples/api/timestamp_queries/README.html):

1. Support for timestamps is detected by comparing VkPhysicalDeviceLimits::timestampPeriod to zero. If it is zero, the device is considered to not support timestamps. I was trying to find support for this idea in Vulkan specification, but found none. The only base for the timestamp support in the spec is based on timestampComputeAndGraphics and timestampValidBits in the spec. There is no word about VkPhysicalDeviceLimits::timestampPeriod value if timestamps are not supported.
2. In the part "Interpreting the results", following equation is used float delta_in_ms = float(time_stamps[1] - time_stamps[0]) * device_limits.timestampPeriod / 1000000.0f;. The equation does not handle timestamp overflows correctly, in my opinion. Timestamp precision is between 36..64 bits. And returned timestamps contain zeros on the place of bits outside of the valid bits range. So, if time_stamps[1] already wraps over to value zero and time_stamps[0] contains maximum value of 36 bits of precision, we have 0x0000'0000'0000'0000 - 0x0000'000f'ffff'ffff = 0xffff'fff0'0000'0001. In other words, the result of subtraction is now not 1 but extremely high number. I guess that correct computation shall use the mask uint64_t timestampValidBitMask = (timestampValidBits >= 64) ? ~uint64_t(0) : (uint64_t(1) << timestampValidBits) - 1;
. And the fixed equation would be float delta_in_ms = float((time_stamps[1] - time_stamps[0]) & timestampValidBitMask) * device_limits.timestampPeriod / 1000000.0f;.
3. The text uses both "timestamp" and "time stamp". The spec uses only "timestamp". I am inclined to suggest to be consistent with the spec.

If I am mistaken, especially on point 1 and 2, please correct me.

Edit: Corrected computation of timestampValidBitMask.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.