Hi, As the title suggests, creating a VkImageView with a format of VK_FORMAT_R8_UNORM out of a VkImage with format VK_FORMAT_G8_B8_R8_3PLANE_420_UNORM (and the mutable flag enabled too) and then using the image as a storage image (e.g. layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; inside a GLSL shader) will result in vec4(0.0f) actually being read instead of the pixel values inside the image. Writing to such images however seems to be successful. I've uploaded some code to replicate this, though I'm afraid its not a small example. The repo is https://github.com/atomnuker/FFmpeg/tree/exp_vulkan. To compile run ./configure with --enable-vulkan. The shader is in libavfilter/vulkan/unsharp.comp and the code which creates the imageviews is in libavfilter/vf_unsharp_vulkan. To run, "./ffmpeg_g -init_hw_device "vulkan=vk:0" -i <some sane video or image> -filter_hw_device vk -vf format=yuv420p,scale=1280x720,hwupload,unsharp_vulkan,hwdownload,format=yuv420p -y out.mkv". I don't have glslang hooked up to the build system yet so you need to regenerate the .h file with "glslangValidator -V unsharp.comp --variable-name "unsharp_vulkan_comp_spv" -o unsharp.comp.h".
I think the mistake here is that you're using a single image with 2 different view. This requires a compatible format between the 2 views. R8 & G8_B8_R8_3PLANE_420 are not compatible. The feature you want to use is VK_IMAGE_CREATE_ALIAS_BIT. You'll need to create 2 image & 2 views and have the 2 images share the same memory. This is a bit confusing to be fair :) I had to reread the spec, even I implemented this... Consider running your application with the validation layers, it'll help spotting potential mistakes like this.
As far as I can tell there isn't a bug here. Closing for now. Feel free to reopen if you find another issue while experimenting with the driver.
(In reply to Lionel Landwerlin from comment #1) > I think the mistake here is that you're using a single image with 2 > different view. This requires a compatible format between the 2 views. R8 & > G8_B8_R8_3PLANE_420 are not compatible. Yes they are. Read this. It's right at the top. https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#features-formats-compatible-planes > > The feature you want to use is VK_IMAGE_CREATE_ALIAS_BIT. You'll need to > create 2 image & 2 views and have the 2 images share the same memory. It's a solution, though not the one I want, is recommended by the spec or something I'd like to waste time just to find out it doesn't work. > > This is a bit confusing to be fair :) I had to reread the spec, even I > implemented this... That would explain why it doesn't work. > > Consider running your application with the validation layers, it'll help > spotting potential mistakes like this. I am running it through validation layers every time and I've reported all of the issues on multiplane images, including the one where they don't list R8 and G8_B8_R8 as compatible. It'll get fixed. Though considering how many assertions have been made in the validation layers that no one will ever bother with multiplane images I'm getting tired. Though the fact that they're slowly getting fixed one at a time gives me hope.
(In reply to atomnuker from comment #3) > (In reply to Lionel Landwerlin from comment #1) > > This is a bit confusing to be fair :) I had to reread the spec, even I > > implemented this... > > That would explain why it doesn't work. Let's keep this civil please.
(In reply to atomnuker from comment #3) > (In reply to Lionel Landwerlin from comment #1) > > I think the mistake here is that you're using a single image with 2 > > different view. This requires a compatible format between the 2 views. R8 & > > G8_B8_R8_3PLANE_420 are not compatible. > > Yes they are. Read this. It's right at the top. > https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec. > html#features-formats-compatible-planes > > > > > > The feature you want to use is VK_IMAGE_CREATE_ALIAS_BIT. You'll need to > > create 2 image & 2 views and have the 2 images share the same memory. > > It's a solution, though not the one I want, is recommended by the spec or > something I'd like to waste time just to find out it doesn't work. > One thing that was also missing on the traces is that no image seem to be created with VK_IMAGE_USAGE_STORAGE_BIT, which what is needed for descriptors of type VK_DESCRIPTOR_TYPE_STORAGE_IMAGE. That seems to hit an assert in my debug build. > > > > > This is a bit confusing to be fair :) I had to reread the spec, even I > > implemented this... > > That would explain why it doesn't work. > > > > > > Consider running your application with the validation layers, it'll help > > spotting potential mistakes like this. > > I am running it through validation layers every time and I've reported all > of the issues on multiplane images, including the one where they don't list > R8 and G8_B8_R8 as compatible. It'll get fixed. > Though considering how many assertions have been made in the validation > layers that no one will ever bother with multiplane images I'm getting > tired. Though the fact that they're slowly getting fixed one at a time gives > me hope.
(In reply to atomnuker from comment #3) > (In reply to Lionel Landwerlin from comment #1) > > I think the mistake here is that you're using a single image with 2 > > different view. This requires a compatible format between the 2 views. R8 & > > G8_B8_R8_3PLANE_420 are not compatible. > > Yes they are. Read this. It's right at the top. > https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec. > html#features-formats-compatible-planes > > As far as I can tell, compatible formats apply to images, not image views. Here is the bit of image creation that refers to compatible formats for planes : " VK_IMAGE_CREATE_ALIAS_BIT indicates that two images created with the same creation parameters and aliased to the same memory can interpret the contents of the memory consistently with each other, subject to the rules described in the Memory Aliasing section. This flag further indicates that each plane of a disjoint image can share an in-memory non-linear representation with single-plane images, and that a single-plane image can share an in-memory non-linear representation with a plane of a multi-planar disjoint image, according to the rules in Compatible formats of planes of multi-planar formats. If the pNext chain includes a VkExternalMemoryImageCreateInfo structure whose handleTypes member is not 0, it is as if VK_IMAGE_CREATE_ALIAS_BIT is set. "
(In reply to Lionel Landwerlin from comment #6) > (In reply to atomnuker from comment #3) > > (In reply to Lionel Landwerlin from comment #1) > > > I think the mistake here is that you're using a single image with 2 > > > different view. This requires a compatible format between the 2 views. R8 & > > > G8_B8_R8_3PLANE_420 are not compatible. > > > > Yes they are. Read this. It's right at the top. > > https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec. > > html#features-formats-compatible-planes > > > > > > As far as I can tell, compatible formats apply to images, not image views. They do: https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VkImageViewCreateInfo >If image was created with the VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT and the image has >a multi-planar format, and if subresourceRange.aspectMask is VK_IMAGE_ASPECT_PLANE_0_BIT, >VK_IMAGE_ASPECT_PLANE_1_BIT, or VK_IMAGE_ASPECT_PLANE_2_BIT, format must be compatible >with the corresponding plane of the image, and the sampler to be used with the image view >must not enable sampler Y’CBCR conversion. The width and height of the single-plane image >view must be derived from the multi-planar image’s dimensions in the manner listed for >plane compatibility for the plane. >One thing that was also missing on the traces is that no image seem to be created with >VK_IMAGE_USAGE_STORAGE_BIT, which what is needed for descriptors of type >VK_DESCRIPTOR_TYPE_STORAGE_IMAGE. That seems to hit an assert in my debug build. Hmm, seems like you're right. Once I flag the bit it works. However, that bit isn't flagged as supported in either linearly or optimally tiled images (of multiplane format). Should this be flagged as supported? It seems like it works.
(In reply to atomnuker from comment #7) > (In reply to Lionel Landwerlin from comment #6) > > (In reply to atomnuker from comment #3) > > > (In reply to Lionel Landwerlin from comment #1) > > > > I think the mistake here is that you're using a single image with 2 > > > > different view. This requires a compatible format between the 2 views. R8 & > > > > G8_B8_R8_3PLANE_420 are not compatible. > > > > > > Yes they are. Read this. It's right at the top. > > > https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec. > > > html#features-formats-compatible-planes > > > > > > > > > > As far as I can tell, compatible formats apply to images, not image views. > > They do: > https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec. > html#VkImageViewCreateInfo > >If image was created with the VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT and the image has > >a multi-planar format, and if subresourceRange.aspectMask is VK_IMAGE_ASPECT_PLANE_0_BIT, > >VK_IMAGE_ASPECT_PLANE_1_BIT, or VK_IMAGE_ASPECT_PLANE_2_BIT, format must be compatible > >with the corresponding plane of the image, and the sampler to be used with the image view > >must not enable sampler Y’CBCR conversion. The width and height of the single-plane image > >view must be derived from the multi-planar image’s dimensions in the manner listed for > >plane compatibility for the plane. > > > >One thing that was also missing on the traces is that no image seem to be created with > >VK_IMAGE_USAGE_STORAGE_BIT, which what is needed for descriptors of type > >VK_DESCRIPTOR_TYPE_STORAGE_IMAGE. That seems to hit an assert in my debug build. > Hmm, seems like you're right. Once I flag the bit it works. However, that > bit isn't flagged as supported > in either linearly or optimally tiled images (of multiplane format). We don't support storage on multiplanar formats indeed. But R8_UNORM should be reported as supporting storage. > Should this be flagged as supported? It seems like it works.
(In reply to Lionel Landwerlin from comment #8) > > We don't support storage on multiplanar formats indeed. > > But R8_UNORM should be reported as supporting storage. > The latter (and RGBA) does indeed but since you're attaching the original multiplane image to the descriptor (albeit indirectly though a derived imageview) its that VkFormat that needs to have STORAGE bit supported. It already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so I guess its just a matter of flagging STORAGE as well.
(In reply to atomnuker from comment #9) > (In reply to Lionel Landwerlin from comment #8) > > > > We don't support storage on multiplanar formats indeed. > > > > But R8_UNORM should be reported as supporting storage. > > > > The latter (and RGBA) does indeed but since you're attaching the original > multiplane image to the descriptor (albeit indirectly though a derived > imageview) its that VkFormat that needs to have STORAGE bit supported. It > already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so > I guess its just a matter of flagging STORAGE as well. It's more or less luck that it works for this particular couple of formats. For something like G8B8G8R8_422_UNORM the hardware really can't do it. So I don't think we want to enable storage on anything ycbcr. Rereading the shader from your repo : #version 460 layout (local_size_x = 16, local_size_y = 16) in; layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img; void main() { vec4 result = imageLoad(input_img, ivec2(gl_GlobalInvocationID.xy)); imageStore(output_img, ivec2(gl_GlobalInvocationID.xy), result); } I'm not sure I understand what is going on here. Is that you're reading from one texture and writing to the same one? Regardless of what texture you access, I think what would work is to have a sampler for the ycbcr texture as input, then you can store to anything that supports storage.
(In reply to Lionel Landwerlin from comment #10) > (In reply to atomnuker from comment #9) > > (In reply to Lionel Landwerlin from comment #8) > > > > > > We don't support storage on multiplanar formats indeed. > > > > > > But R8_UNORM should be reported as supporting storage. > > > > > > > The latter (and RGBA) does indeed but since you're attaching the original > > multiplane image to the descriptor (albeit indirectly though a derived > > imageview) its that VkFormat that needs to have STORAGE bit supported. It > > already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so > > I guess its just a matter of flagging STORAGE as well. > > It's more or less luck that it works for this particular couple of formats. > For something like G8B8G8R8_422_UNORM the hardware really can't do it. > So I don't think we want to enable storage on anything ycbcr. Yes you can. Its listed right there in the compatible formats. Check out the earlier link I posted. 422 images support VkImageView of R8 for the luma plane and R8G8 for the chroma planes (so you get vec2() for the chroma plane of such images). In my use case I need to be able to do this for all YUV images, since otherwise they'd be pretty much useless. And you can do that with all YUV images like it says in the spec. > > Rereading the shader from your repo : > > #version 460 > > layout (local_size_x = 16, local_size_y = 16) in; > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img; > > void main() > { > vec4 result = imageLoad(input_img, ivec2(gl_GlobalInvocationID.xy)); > imageStore(output_img, ivec2(gl_GlobalInvocationID.xy), result); > } > > I'm not sure I understand what is going on here. Is that you're reading from > one texture and writing to the same one? > > Regardless of what texture you access, I think what would work is to have a > sampler for the ycbcr texture as input, then you can store to anything that > supports storage. I don't need to sample for this particular case. Sampling ought to be slower as well. I just need to fetch individual pixels without interpolation. And like I said, this works as long as you disrespect what the driver currently reports and flag the STORAGE bit when creating the image. So its a matter of marking it as supported.
(In reply to atomnuker from comment #11) > (In reply to Lionel Landwerlin from comment #10) > > (In reply to atomnuker from comment #9) > > > (In reply to Lionel Landwerlin from comment #8) > > > > > > > > We don't support storage on multiplanar formats indeed. > > > > > > > > But R8_UNORM should be reported as supporting storage. > > > > > > > > > > The latter (and RGBA) does indeed but since you're attaching the original > > > multiplane image to the descriptor (albeit indirectly though a derived > > > imageview) its that VkFormat that needs to have STORAGE bit supported. It > > > already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so > > > I guess its just a matter of flagging STORAGE as well. > > > > It's more or less luck that it works for this particular couple of formats. > > For something like G8B8G8R8_422_UNORM the hardware really can't do it. > > So I don't think we want to enable storage on anything ycbcr. > Yes you can. Its listed right there in the compatible formats. Check out the > earlier link I posted. 422 images support VkImageView of R8 for the luma > plane and R8G8 for the chroma planes (so you get vec2() for the chroma plane > of such images). In my use case I need to be able to do this for all YUV > images, since otherwise they'd be pretty much useless. And you can do that > with all YUV images like it says in the spec. > > > > > > Rereading the shader from your repo : > > > > #version 460 > > > > layout (local_size_x = 16, local_size_y = 16) in; > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img; > > > > void main() > > { > > vec4 result = imageLoad(input_img, ivec2(gl_GlobalInvocationID.xy)); > > imageStore(output_img, ivec2(gl_GlobalInvocationID.xy), result); If you swizzle the result here, I'm pretty sure you won't get what you expect. Hopefully that convinces you that this isn't supported. > > } > > > > I'm not sure I understand what is going on here. Is that you're reading from > > one texture and writing to the same one? > > > > Regardless of what texture you access, I think what would work is to have a > > sampler for the ycbcr texture as input, then you can store to anything that > > supports storage. > I don't need to sample for this particular case. Sampling ought to be slower > as well. I just need to fetch individual pixels without interpolation. And > like I said, this works as long as you disrespect what the driver currently > reports and flag the STORAGE bit when creating the image. So its a matter of > marking it as supported.
(In reply to Lionel Landwerlin from comment #12) > (In reply to atomnuker from comment #11) > > (In reply to Lionel Landwerlin from comment #10) > > > (In reply to atomnuker from comment #9) > > > > (In reply to Lionel Landwerlin from comment #8) > > > > > > > > > > We don't support storage on multiplanar formats indeed. > > > > > > > > > > But R8_UNORM should be reported as supporting storage. > > > > > > > > > > > > > The latter (and RGBA) does indeed but since you're attaching the original > > > > multiplane image to the descriptor (albeit indirectly though a derived > > > > imageview) its that VkFormat that needs to have STORAGE bit supported. It > > > > already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so > > > > I guess its just a matter of flagging STORAGE as well. > > > > > > It's more or less luck that it works for this particular couple of formats. > > > For something like G8B8G8R8_422_UNORM the hardware really can't do it. > > > So I don't think we want to enable storage on anything ycbcr. > > Yes you can. Its listed right there in the compatible formats. Check out the > > earlier link I posted. 422 images support VkImageView of R8 for the luma > > plane and R8G8 for the chroma planes (so you get vec2() for the chroma plane > > of such images). In my use case I need to be able to do this for all YUV > > images, since otherwise they'd be pretty much useless. And you can do that > > with all YUV images like it says in the spec. I should correct myself, this is for NV12. For 422 you use the same procedure as 420 but with the image resolution being halved horizontally. > > > > > > > > > > Rereading the shader from your repo : > > > > > > #version 460 > > > > > > layout (local_size_x = 16, local_size_y = 16) in; > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img; > > > > > > void main() > > > { > > > vec4 result = imageLoad(input_img, ivec2(gl_GlobalInvocationID.xy)); > > > imageStore(output_img, ivec2(gl_GlobalInvocationID.xy), result); > > If you swizzle the result here, I'm pretty sure you won't get what you > expect. > Hopefully that convinces you that this isn't supported. Well of course it won't work, this copies 1 plane only. For multiplane images I make both the output images and input images arrays and I iterate over them. The result vec4 only contains 1 pixel value in the .r member. Like the spec says, creating a single plane imageview from a multiplane format exposes 1 plane and 1 plane only. I have pushed a new version to the same repository which works for 420, 422 and 444 images. Tested. It also works for NV12 images with minimal modifications (iterating over 2 planes instead of 3 and with different format on the second plane). If you have nothing more to argue about then I propose we end this conversation and instead focus on fixing the flags.
(In reply to atomnuker from comment #13) > (In reply to Lionel Landwerlin from comment #12) > > (In reply to atomnuker from comment #11) > > > (In reply to Lionel Landwerlin from comment #10) > > > > (In reply to atomnuker from comment #9) > > > > > (In reply to Lionel Landwerlin from comment #8) > > > > > > > > > > > > We don't support storage on multiplanar formats indeed. > > > > > > > > > > > > But R8_UNORM should be reported as supporting storage. > > > > > > > > > > > > > > > > The latter (and RGBA) does indeed but since you're attaching the original > > > > > multiplane image to the descriptor (albeit indirectly though a derived > > > > > imageview) its that VkFormat that needs to have STORAGE bit supported. It > > > > > already supports the SAMPLE bit (needed for YUV->RGB converting samplers) so > > > > > I guess its just a matter of flagging STORAGE as well. > > > > > > > > It's more or less luck that it works for this particular couple of formats. > > > > For something like G8B8G8R8_422_UNORM the hardware really can't do it. > > > > So I don't think we want to enable storage on anything ycbcr. > > > Yes you can. Its listed right there in the compatible formats. Check out the > > > earlier link I posted. 422 images support VkImageView of R8 for the luma > > > plane and R8G8 for the chroma planes (so you get vec2() for the chroma plane > > > of such images). In my use case I need to be able to do this for all YUV > > > images, since otherwise they'd be pretty much useless. And you can do that > > > with all YUV images like it says in the spec. > > I should correct myself, this is for NV12. For 422 you use the same > procedure as 420 but with the image resolution being halved horizontally. > > > > > > > > > > > > > > > Rereading the shader from your repo : > > > > > > > > #version 460 > > > > > > > > layout (local_size_x = 16, local_size_y = 16) in; > > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img; > > > > > > > > void main() > > > > { > > > > vec4 result = imageLoad(input_img, ivec2(gl_GlobalInvocationID.xy)); > > > > imageStore(output_img, ivec2(gl_GlobalInvocationID.xy), result); > > > > If you swizzle the result here, I'm pretty sure you won't get what you > > expect. > > Hopefully that convinces you that this isn't supported. > > Well of course it won't work, this copies 1 plane only. For multiplane > images I make both the output images and input images arrays and I iterate > over them. The result vec4 only contains 1 pixel value in the .r member. > Like the spec says, creating a single plane imageview from a multiplane > format exposes 1 plane and 1 plane only. The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 image. layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; But somehow it should contain all the planes? > > I have pushed a new version to the same repository which works for 420, 422 > and 444 images. Tested. It also works for NV12 images with minimal > modifications (iterating over 2 planes instead of 3 and with different > format on the second plane). > > If you have nothing more to argue about then I propose we end this > conversation and instead focus on fixing the flags.
(In reply to Lionel Landwerlin from comment #14) > The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 > image. Yes, the one before I pushed. I only used 1 plane in order to test easily without modifying lots of code. > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > But somehow it should contain all the planes? Like I said, no, just one: the one with the aspect (plane) you chose via the VkImageView. To illustrate: VkImage (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR) -> create VkImageView (VK_FORMAT_R8_UNORM and aspect VK_IMAGE_ASPECT_PLANE_0_BIT_KHR) -> bind to descriptor which describes a storage image inside the shader -> imageLoad(input_img, vec2(0)).r will output the pixel of the image at location x=0, y=0. To have access to all planes you need to create separate imageviews and bind them to different images inside the shader, like: > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img[3]; > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img[3]; When you do a read into one of the source images you'll get the pixel value inside the .r component of the resulting vec4. The rest of the components contain 0.0f, as expected when the format contains 1 component.
(In reply to atomnuker from comment #15) > (In reply to Lionel Landwerlin from comment #14) > > The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 > > image. > Yes, the one before I pushed. I only used 1 plane in order to test easily > without modifying lots of code. > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > > But somehow it should contain all the planes? > Like I said, no, just one: the one with the aspect (plane) you chose via the > VkImageView. > > To illustrate: > VkImage (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR) -> create VkImageView > (VK_FORMAT_R8_UNORM and aspect VK_IMAGE_ASPECT_PLANE_0_BIT_KHR) -> bind to > descriptor which describes a storage image inside the shader -> > imageLoad(input_img, vec2(0)).r will output the pixel of the image at > location x=0, y=0. We can't do that, because enabling storage support on a VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR image means we would need to support that storage on that entire format too (not just on one plane). > > To have access to all planes you need to create separate imageviews and bind > them to different images inside the shader, like: > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img[3]; > > layout (set = 0, binding = 1, rgba8) uniform writeonly image2D output_img[3]; > > When you do a read into one of the source images you'll get the pixel value > inside the .r component of the resulting vec4. The rest of the components > contain 0.0f, as expected when the format contains 1 component.
(In reply to Lionel Landwerlin from comment #16) > (In reply to atomnuker from comment #15) > > (In reply to Lionel Landwerlin from comment #14) > > > The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 > > > image. > > Yes, the one before I pushed. I only used 1 plane in order to test easily > > without modifying lots of code. > > > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > > > > But somehow it should contain all the planes? > > Like I said, no, just one: the one with the aspect (plane) you chose via the > > VkImageView. > > > > To illustrate: > > VkImage (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR) -> create VkImageView > > (VK_FORMAT_R8_UNORM and aspect VK_IMAGE_ASPECT_PLANE_0_BIT_KHR) -> bind to > > descriptor which describes a storage image inside the shader -> > > imageLoad(input_img, vec2(0)).r will output the pixel of the image at > > location x=0, y=0. > > We can't do that, because enabling storage support on a > VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR image means we would need to support > that storage on that entire format too (not just on one plane). We must: the spec explicitly states that API users MUST check if the VkFormat supports the STORAGE bit for the current tiling mode. If its not flagged, then the STORAGE usage bit MUST NOT be present when creating the image. If the storage bit isn't enabled then any VkImageViews created from the VkImage will not support storage, even if the VkFormat of the VkImageView supports the STORAGE bit for the particular tiling mode. Hence, we must flag the STORAGE bit for the VkFormat, even if users can't directly access all the planes via a single image2D. I can propose that the spec is changed such that it mentions this, if it makes you feel more comfortable. But to move forward and enable users to do _actual_ useful things to multiplane images (and not take chances by flagging usages reported to be unsupported) we should flag this.
(In reply to atomnuker from comment #17) > (In reply to Lionel Landwerlin from comment #16) > > (In reply to atomnuker from comment #15) > > > (In reply to Lionel Landwerlin from comment #14) > > > > The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 > > > > image. > > > Yes, the one before I pushed. I only used 1 plane in order to test easily > > > without modifying lots of code. > > > > > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > > > > > > But somehow it should contain all the planes? > > > Like I said, no, just one: the one with the aspect (plane) you chose via the > > > VkImageView. > > > > > > To illustrate: > > > VkImage (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR) -> create VkImageView > > > (VK_FORMAT_R8_UNORM and aspect VK_IMAGE_ASPECT_PLANE_0_BIT_KHR) -> bind to > > > descriptor which describes a storage image inside the shader -> > > > imageLoad(input_img, vec2(0)).r will output the pixel of the image at > > > location x=0, y=0. > > > > We can't do that, because enabling storage support on a > > VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR image means we would need to support > > that storage on that entire format too (not just on one plane). > > We must: the spec explicitly states that API users MUST check if the > VkFormat supports the STORAGE bit for the current tiling mode. If its not > flagged, then the STORAGE usage bit MUST NOT be present when creating the > image. If the storage bit isn't enabled then any VkImageViews created from > the VkImage will not support storage, even if the VkFormat of the > VkImageView supports the STORAGE bit for the particular tiling mode. > > Hence, we must flag the STORAGE bit for the VkFormat, even if users can't > directly access all the planes via a single image2D. I can propose that the > spec is changed such that it mentions this, if it makes you feel more > comfortable. But to move forward and enable users to do _actual_ useful > things to multiplane images (and not take chances by flagging usages > reported to be unsupported) we should flag this. If you get the spec changed, I'll be happy to add the storage bit on multi-planar formats. As it stands though, supporting storage on multi-planar (in all possible cases) is a few changes not really worth making since you can avoid it with just an additional VkImage.
(In reply to Lionel Landwerlin from comment #18) > (In reply to atomnuker from comment #17) > > (In reply to Lionel Landwerlin from comment #16) > > > (In reply to atomnuker from comment #15) > > > > (In reply to Lionel Landwerlin from comment #14) > > > > > The ffmpeg repo I built locally had the image below bound to a G8_B8_R8 > > > > > image. > > > > Yes, the one before I pushed. I only used 1 plane in order to test easily > > > > without modifying lots of code. > > > > > > > > > layout (set = 0, binding = 0, rgba8) uniform readonly image2D input_img; > > > > > > > > > > But somehow it should contain all the planes? > > > > Like I said, no, just one: the one with the aspect (plane) you chose via the > > > > VkImageView. > > > > > > > > To illustrate: > > > > VkImage (VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR) -> create VkImageView > > > > (VK_FORMAT_R8_UNORM and aspect VK_IMAGE_ASPECT_PLANE_0_BIT_KHR) -> bind to > > > > descriptor which describes a storage image inside the shader -> > > > > imageLoad(input_img, vec2(0)).r will output the pixel of the image at > > > > location x=0, y=0. > > > > > > We can't do that, because enabling storage support on a > > > VK_FORMAT_G8_B8R8_2PLANE_420_UNORM_KHR image means we would need to support > > > that storage on that entire format too (not just on one plane). > > > > We must: the spec explicitly states that API users MUST check if the > > VkFormat supports the STORAGE bit for the current tiling mode. If its not > > flagged, then the STORAGE usage bit MUST NOT be present when creating the > > image. If the storage bit isn't enabled then any VkImageViews created from > > the VkImage will not support storage, even if the VkFormat of the > > VkImageView supports the STORAGE bit for the particular tiling mode. > > > > Hence, we must flag the STORAGE bit for the VkFormat, even if users can't > > directly access all the planes via a single image2D. I can propose that the > > spec is changed such that it mentions this, if it makes you feel more > > comfortable. But to move forward and enable users to do _actual_ useful > > things to multiplane images (and not take chances by flagging usages > > reported to be unsupported) we should flag this. > > If you get the spec changed, I'll be happy to add the storage bit on > multi-planar formats. > As it stands though, supporting storage on multi-planar (in all possible > cases) is a few changes not really worth making since you can avoid it with > just an additional VkImage. Which is an ugly hack which relies that the memory bound to both images is properly aligned, has the same tiling and is otherwise idential in all respects. Its awful. I don't know how you can even propose this to be a valid workaround. You do realize that you're contradicting and rejecting a part of the specifications? And until today I don't think you had even heard of this despite stating that you wrote a part of it. I'd like to have another person's opinion on this, preferably someone who actually reads the spec.
If anybody is looking at this, the 2 images suggestion is tested in the CTS by the dEQP-VK.ycbcr.plane_view.memory_alias.* tests.
> > If you get the spec changed, I'll be happy to add the storage bit on > > multi-planar formats. > > As it stands though, supporting storage on multi-planar (in all possible > > cases) is a few changes not really worth making since you can avoid it with > > just an additional VkImage. > > Which is an ugly hack which relies that the memory bound to both images is > properly aligned, has the same tiling and is otherwise idential in all > respects. Its awful. I don't know how you can even propose this to be a > valid workaround. Yes, just creating a view seems to be nicer than creating an aliasing image. However, I don't think that's as ugly as you think it is. If you create the image with the disjoint bit, it is required to work as per section 11.8 of the Vulkan 1.1.71 spec. > You do realize that you're contradicting and rejecting a > part of the specifications? And until today I don't think you had even heard > of this despite stating that you wrote a part of it. > I'd like to have another person's opinion on this, preferably someone who > actually reads the spec. I realize that this discussion is rather frustrating. However, that's not a valid excuse for your belligerence. We can have a civil technical discussion without making any accusations. Lionel is vary knowledgeable both about the YcBcR spec and our driver and should be treated as such. If there is some bit of the spec you believe he is missing, feel free to quote it and make your argument but please drop the hostility.
Reading through things, I believe what you are trying to do is possible as per the spec but it requires a bit more care than you have given it. In particular, the YCbCr image needs to be created with the VK_IMAGE_CREATE_EXTENDED_USAGE_BIT and you need to chain in a VkImageViewUsageCreateInfo struct to vkCreateImageView with the VK_IMAGE_USAGE_STORAGE_BIT set. That said, the interaction here between VK_KHR_maintenance2 and VK_KHR_sampler_ycbcr_conversion is a bit subtle so I'm not that surprised that both of you (along with the CTS authors) missed it.
Digging through the khronos gitlab, it seems the extended flags was added for another purpose : https://gitlab.khronos.org/vulkan/vulkan/merge_requests/1896 I think the usage you describe looks like it would fix atomnuker's problem, but it feels like an unintended consequence :) VK_FORMAT_R8_UNORM being a required format for with storage support, we could add a CTS test to verify that with ycbcr formats (adding that to my todo list).
(In reply to Lionel Landwerlin from comment #23) > Digging through the khronos gitlab, it seems the extended flags was added > for another purpose : > https://gitlab.khronos.org/vulkan/vulkan/merge_requests/1896 > > I think the usage you describe looks like it would fix atomnuker's problem, > but it feels like an unintended consequence :) Well, the EXTENDED usage flag is for when your imageview's format is supported by the GPU but the format the image was created with is not. It only applies to formats, rather than usage flags, tiling, etc. The flag to be able to create single-plane representations of multi-plane images is VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT. > That said, the interaction here between VK_KHR_maintenance2 and > VK_KHR_sampler_ycbcr_conversion is a bit subtle so I'm not that > surprised that both of you (along with the CTS authors) missed it. There should be no interaction between either, maintenance2 isn't required by the YCBCR extension, but maintenance1 is. > Yes, just creating a view seems to be nicer than creating an aliasing image. > However, I don't think that's as ugly as you think it is. If you create the > image with the disjoint bit, it is required to work as per section 11.8 of > the Vulkan 1.1.71 spec. Well, it sort of is - having the DISJOINT bit set on an image you created means having to allocate each plane separately, contributing to memory fragmentation, allocation time, flushing and invalidating, and, depending on implementation, image accessing speed. Anyway, I've submitted a proposal to clarify that the STORAGE usage flag on multiplanar formats only applies to single-plane VKImageViews. Knowing that another implementation (nvidia's) also doesn't flag the storage bit for multiplanar images makes me thing I should word the proposal more strongly (e.g. if all compatible singleplane representations' formats supports the STORAGE bit, then it must be set as well for the multiplanar format).
> > > Anyway, I've submitted a proposal to clarify that the STORAGE usage flag on > multiplanar formats only applies to single-plane VKImageViews. Knowing that > another implementation (nvidia's) also doesn't flag the storage bit for > multiplanar images makes me thing I should word the proposal more strongly > (e.g. if all compatible singleplane representations' formats supports the > STORAGE bit, then it must be set as well for the multiplanar format). If you have a link to your proposal, please put it here, I would like to follow it. Thanks.
(In reply to atomnuker from comment #24) > (In reply to Lionel Landwerlin from comment #23) > > Digging through the khronos gitlab, it seems the extended flags was added > > for another purpose : > > https://gitlab.khronos.org/vulkan/vulkan/merge_requests/1896 > > > > I think the usage you describe looks like it would fix atomnuker's problem, > > but it feels like an unintended consequence :) > > Well, the EXTENDED usage flag is for when your imageview's format is > supported by the GPU but the format the image was created with is not. It > only applies to formats, rather than usage flags, tiling, etc. > The flag to be able to create single-plane representations of multi-plane > images is VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT. That's utter nonsense. It exists precisely for cases like this where to have an image in a format that supports some limited usage flagss but you want to create a view of it in a compatible format has greater usage flags which you want to take advantage of. Originally, the purpose was to combine it with another flash and allow creating uncompressed views of compressed textures so that you can write a texture compressor in a compute shader. However, it also seems to apply quite nicely here. > > That said, the interaction here between VK_KHR_maintenance2 and > > VK_KHR_sampler_ycbcr_conversion is a bit subtle so I'm not that > > surprised that both of you (along with the CTS authors) missed it. > > There should be no interaction between either, maintenance2 isn't required > by the YCBCR extension, but maintenance1 is. Depending upon and interacting are two totally different things. Perhaps the interaction should be called out in a note out something but there's no reason they can't interact. > Anyway, I've submitted a proposal to clarify that the STORAGE usage flag on > multiplanar formats only applies to single-plane VKImageViews. Knowing that > another implementation (nvidia's) also doesn't flag the storage bit for > multiplanar images makes me thing I should word the proposal more strongly > (e.g. if all compatible singleplane representations' formats supports the > STORAGE bit, then it must be set as well for the multiplanar format). I highly doubt that proposal will fly. An implementation advertising STORAGE for a planner format means that you can store to that planner format. Making usage flags for one format actually imply something about views in another format is a fairly bad abuse of the usage flash system. I can't personally speak for the entire working group (thought I am fairly heavily involved with it) but I strongly suspect that using VK_KHR_IMAGE_CREATE_EXTENDED_USAGE_BIT will be their response.
Closing, will use the EXTENDED image creation flag.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.