Change in has_external_format

[panos-lunarg]

External images which during capture have been dumped as VK_FORMAT_R8G8B8A8_UNORM, during replay can be treated as normal images

[MarkY-LunarG]

I don't see you explicitly checking for R8G8B8A8 where you removed the external copy. Is this not needed?

[panos-lunarg]

I don't see you explicitly checking for R8G8B8A8 where you removed the external copy. Is this not needed?

From what I understand if you are inside this if condition then you are hitting this case where the AHB was sampled in the compute path and dumped as a R8G8B8A8 image. I believe this is confirmed by the other 3 lines above:

// In this case, the image has been sampled at capture time and format is now RGBA8_UNORM.
modified_create_info.format     = VK_FORMAT_R8G8B8A8_UNORM;
external_format->externalFormat = 0;

[MarkY-LunarG]

I don't see you explicitly checking for R8G8B8A8 where you removed the external copy. Is this not needed?

From what I understand if you are inside this if condition then you are hitting this case where the AHB was sampled in the compute path and dumped as a R8G8B8A8 image. I believe this is confirmed by the other 3 lines above:

// In this case, the image has been sampled at capture time and format is now RGBA8_UNORM.
modified_create_info.format     = VK_FORMAT_R8G8B8A8_UNORM;
external_format->externalFormat = 0;

Should we at least add an assert to make sure it doesn't change in the future? It just seems this is a little fragile without some watch.

[panos-lunarg]

I don't see you explicitly checking for R8G8B8A8 where you removed the external copy. Is this not needed?

From what I understand if you are inside this if condition then you are hitting this case where the AHB was sampled in the compute path and dumped as a R8G8B8A8 image. I believe this is confirmed by the other 3 lines above:

// In this case, the image has been sampled at capture time and format is now RGBA8_UNORM.
modified_create_info.format     = VK_FORMAT_R8G8B8A8_UNORM;
external_format->externalFormat = 0;

Should we at least add an assert to make sure it doesn't change in the future? It just seems this is a little fragile without some watch.

Sorry I am a bit confused, can you be more specific about to what to check with an assert? VK_FORMAT_R8G8B8A8_UNORM is not stored somewhere, if the if condition is true then we assign the format to the new image.

[MarkY-LunarG]

Sorry I am a bit confused, can you be more specific about to what to check with an assert? VK_FORMAT_R8G8B8A8_UNORM is not stored somewhere, if the if condition is true then we assign the format to the new image.

But only if the external_format is originally undefined. It seems to me we could enter this same path without that being true, and what guarantee do we have that the original format is RGBA8? That's my concern. I won't hold up the review for this, I just worry about what could happen.

[panos-lunarg]

and what guarantee do we have that the original format is RGBA8?

This is basically a silent "agreement" between capture at

gfxreconstruct/framework/encode/vulkan_capture_common.cpp

Line 473 in a38575b

host_image_format = VK_FORMAT_R8G8B8A8_UNORM;

and this place