-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add fixup for image1dbuffer with bgra format on Intel #629
Conversation
4111013
to
7ef22ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change! Good to start extracting all the image tables, etc.
7ef22ad
to
e88f757
Compare
e88f757
to
8e74ac8
Compare
auto image_channel_order = mapping.first.image_channel_order; | ||
auto image_channel_data_type = mapping.first.image_channel_data_type; | ||
VkComponentMapping components_sampled, components_storage; | ||
image_format_support fmt_support; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping.second
if (!cl_image_format_to_vulkan_format(clfmt, image_type, dev, | ||
&fmt_support, &components_sampled, | ||
&components_storage)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the conversion now that you're iterating over the map again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove this conversion, this PR makes no sense. The whole point of it is this conversion which allows to check for is_bgra_format_not_supported_for_image1d_buffer
. That's why I did not want to iterate over the maps initially. But I agree having a getter is better. But I would prefer not to have access to the VkFormat there as people need to understand that the only mapping to consider is the one from the conversion function, not the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the getter, we could have a function returning the number of elements in the map, and a function to get the cl_image_format
(only) for a specified element:
const unsigned get_format_maps_size();
const cl_image_format get_format_from_format_maps_at(unsigned i);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget about that, FormatMaps is a map... not possible to do it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'd lost track of the big picture :D. I'm sure we'll find good ways of refactoring this as more support for images makes its way into the code.
also fix condition for CL_LUMINANCE and CL_INTENSITY (ref kpet#625)
8e74ac8
to
4b665ea
Compare
if (!cl_image_format_to_vulkan_format(clfmt, image_type, dev, | ||
&fmt_support, &components_sampled, | ||
&components_storage)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'd lost track of the big picture :D. I'm sure we'll find good ways of refactoring this as more support for images makes its way into the code.
also fix condition for CL_LUMINANCE and CL_INTENSITY (ref #625)