layers: GH1752 Update ImageSubrange checks #1758
Conversation
| namespace std { | ||
|
|
||
| template <typename T> | ||
| std::string to_string(T var) { |
There was a problem hiding this comment.
Not entirely happy with you defining stuff in std namespace. This is generally not acceptable, except for adding specializations for your own types.
There was a problem hiding this comment.
It does suck that we don't have it in the NDK yet. Perhaps add a utility call that does the same, using to_string for all platforms except Android?
There was a problem hiding this comment.
It is a hack/workaround.
I want it to be unacceptable and to break the minute Android provides proper STL and this project migrates to that new NDK version.
Also, I already sneaked one of these in core_validation.cpp 😋
Are you fine with it as explained, or should I try a different approach?
There was a problem hiding this comment.
OK, let's go with this as you've written it.
There was a problem hiding this comment.
Hah, okay. Fine with me given precedent, although we'll have to decide if we want to force everyone to whatever NDK eventually supports it. Looks like r16 or later now, per android/ndk#82
| HandleToUint64(image_state->image), __LINE__, VALIDATION_ERROR_00768, "IMAGE", "%s: %s.levelCount is 0. %s", | ||
| cmd_name, param_name, validation_error_map[VALIDATION_ERROR_00768]); | ||
| } else if (subresourceRange.levelCount == VK_REMAINING_MIP_LEVELS) { | ||
| // TODO: Not in the spec VUs. Probably missing -- KhronosGroup/Vulkan-Docs#416 |
There was a problem hiding this comment.
Are we waiting for this to be resolved before wanting to land this?
There was a problem hiding this comment.
Doesn't feel like it should be my decision... but IMO it is an obvious spec bug, and the resolution will not be surprising. Besides it is unspecified behavior even currently (i.e. what would the semantics of this Base>Max with Remaining even be, if allowed) and so worth a diagnostic message. Users can benefit in the meantime (it may be months, it may be days before resolution).
tests/layer_validation_tests.cpp
Outdated
| const int32_t tex_width = 32; | ||
| const int32_t tex_height = 32; | ||
| VkImageCreateInfo image_create_info = {}; | ||
| image_create_info.sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO; |
There was a problem hiding this comment.
Should be able to use VkImageObj to get rid of most of this boilerplate
There was a problem hiding this comment.
Yea.
I just stole this stuff from the test below...
There was a problem hiding this comment.
@chrisforbes Alright, dealt with this + made some other code quality touches to the tests.
tests/layer_validation_tests.cpp
Outdated
| vkCmdClearColorImage(m_commandBuffer->GetBufferHandle(), dstImage.handle(), VK_IMAGE_LAYOUT_GENERAL, &clear_color, 1, | ||
| &range); | ||
| m_errorMonitor->VerifyFound(); | ||
| } |
There was a problem hiding this comment.
Meat of this test looks good 👍
- fix #1752 - handle possible overflow of `level`+`count` - include correct err code in msges - update and add some tests
level+countI had to use the extended VU code + text. The unextended VU is missing in the database (though it has its own VUID in the spec).
Does not include 3D slicing tests (I don't have
VK_KHR_maintenance1anyway).The validation check for 3D slicing is included though. There's not much room for error (it is tied to the normal non 3D code). Someone should check it (and add the tests) just the same.