Vulkan: Cleanup/refactoring, faster RealXFB

[stenzek]

Tidies a decent chunk of code up by moving things around and removing duplication.

It would be good to get some feedback from users experiencing issues with RealXFB previously to test this PR, to see if it changes anything.

In purely XFB-copy based tests, it's significantly faster, but I'd expect the benefit in actual games to be much lower, if any.

It'd be a good idea to test EFB2RAM for regressions as well, I've fixed an issue here where it was creating duplicate entries in the kick-on-draw-list.

---

This change is 

[lioncash]

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/StagingTexture2D.h, line 16 at r1 (raw file):

namespace Vulkan
{
class StagingTexture2D final : public StagingBuffer

If you're forming an inheritance tree, StagingBuffer's destructor should likely be made virtual.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

void TextureConverter::ConvertTexture(TextureCache::TCacheEntry* dst_entry,
                                      TextureCache::TCacheEntry* src_entry,
                                      VkRenderPass render_pass, void* palette,

As far as I can tell, palette can be const void*

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 137 at r1 (raw file):

  // Allocate memory for the palette, and descriptor sets for the buffer.
  // If any of these fail, execute a command buffer, and try again.
  if (!m_texel_buffer->ReserveMemory(palette_size, texel_buffer_alignment))

This if should be its own function.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 158 at r1 (raw file):

  // so we must convert them at the appropriate time, during the drawing command buffer.
  VkCommandBuffer command_buffer;
  if (src_entry->IsEfbCopy())

This if-else should likely be its own function.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 308 at r1 (raw file):

  // Pack each row without any padding in the texel buffer.
  u32 upload_stride = src_width * sizeof(u16);

sizeof returns a size_t.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 611 at r1 (raw file):

                               ENCODING_TEXTURE_HEIGHT, ENCODING_TEXTURE_FORMAT);

  if (!m_encoding_download_texture || !m_encoding_download_texture->Map())

Since the expression and the following returns form the following:

if (cond)
    return false
else
    return true;

you can collapse it into a return like so:

return m_encoding_download_texture && m_encoding_download_texture->Map();

---

Comments from Reviewable

[stenzek]

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/CMakeLists.txt, line 17 at r1 (raw file):

Previously, degasus (Markus Wick) wrote…

> TAB

Done.

---

Source/Core/VideoBackends/Vulkan/StagingTexture2D.h, line 16 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> If you're forming an inheritance tree, `StagingBuffer`'s destructor should likely be made virtual.

Indeed. StagingBuffer wasn't originally intended to be a base class.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> As far as I can tell, palette can be `const void*`

Done.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 137 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> This if should be its own function.

Done.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 158 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> This if-else should likely be its own function.

Done.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 308 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> sizeof returns a size_t.

Done.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 611 at r1 (raw file):

Previously, lioncash (Mat M.) wrote…

> Since the expression and the following returns form the following: > > ```cpp > if (cond) > return false > else > return true; > ``` > > you can collapse it into a return like so: > > ```cpp > return m_encoding_download_texture && m_encoding_download_texture->Map(); > ```

Done.

---

Comments from Reviewable

[linkmauve]

This PR fixes the scanner visor in Metroid Prime 2: Echoes.

[iwubcode]

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

Previously, stenzek (Stenzek) wrote…

Done.

I know void* is necessary at times, so my apologies if this is one of those times, but does the palette really not have a known type?

---

Source/Core/VideoBackends/Vulkan/Util.cpp, line 641 at r4 (raw file):

  // Fast path when there are no gaps in the set bindings
  u32 bind_point_index;
  for (bind_point_index = 0; bind_point_index < NUM_DESCRIPTOR_SET_BIND_POINTS; bind_point_index++)

I realize that this is a rather small loop but I don't think you'd need the loop at all if the array only contained non-null handles. It'd avoid the check for null in the "remaining sets" loop below too. You already check for null in the above code to raise panic alerts, so it seems making an array with only non-null entries should incur little penalty.

---

Comments from Reviewable

[iwubcode]

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

Previously, iwubcode wrote…

I know void* is necessary at times, so my apologies if this is one of those times, but does the palette really not have a known type?

Sorry, this comment was a bit premature, I see now that TextureConverter's ConvertTexture gets called by TextureCache which is just following an interface..

---

Comments from Reviewable

[stenzek]

Review status: 0 of 25 files reviewed at latest revision, 7 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

Previously, iwubcode wrote…

Sorry, this comment was a bit premature, I see now that TextureConverter's ConvertTexture gets called by TextureCache which is just following an interface..

The palette formats here all fit into 16 bits (IA8, RGB5A3, RGB565), so we could probably use a u16 here as the type. However the bit range for each colour differs between the formats, so using a void type does make some sense, as there is no single way to interpret the data. Perhaps a union would be appropriate, although the host doesn't process the palette at all, it's just copied as-is to the GPU.

Like you said, it is just following the interface, but that doesn't mean the interface can't be changed to make it more clear.

---

Source/Core/VideoBackends/Vulkan/Util.cpp, line 641 at r4 (raw file):

Previously, iwubcode wrote…

I realize that this is a rather small loop but I don't think you'd need the loop at all if the array only contained non-null handles. It'd avoid the check for null in the "remaining sets" loop below too. You already check for null in the above code to raise panic alerts, so it seems making an array with only non-null entries should incur little penalty.

It's probably not clear just by examining the code, but the issue here is that we have three bind points (uniform buffers, sampler, and texel/storage buffers), and you can have any combination of the three (e.g. uniforms and a texel buffer, but no samplers, so no set for the second bind point will be allocated).

vkCmdBindDescriptorSets can't accept a null binding, so binding all 3 sets each time will result in errors for this case, so we have to do it as two bind calls. However, if we had uniforms and samplers, but no texel buffers, we could do this as a single bind command as there is no gap.

The whole function is a bit messy though, it could be definitely be refactored in the future.

---

Comments from Reviewable

[iwubcode]

Review status: 0 of 25 files reviewed at latest revision, 6 unresolved discussions.

---

Source/Core/VideoBackends/Vulkan/TextureConverter.cpp, line 117 at r1 (raw file):

Previously, stenzek (Stenzek) wrote…

The palette formats here all fit into 16 bits (IA8, RGB5A3, RGB565), so we could probably use a u16 here as the type. However the bit range for each colour differs between the formats, so using a void type does make some sense, as there is no single way to interpret the data. Perhaps a union would be appropriate, although the host doesn't process the palette at all, it's just copied as-is to the GPU.

Like you said, it is just following the interface, but that doesn't mean the interface can't be changed to make it more clear.

Thanks for explanation. I see the conundrum! If it is truly not processed just passed to the GPU then I agree, u16 gives more context to me than void*. Regardless, yeah, I figured that an interface change was probably too much for this particular code submission.

---

Comments from Reviewable

[iwubcode]

Reviewed 13 of 25 files at r1, 3 of 4 files at r2, 2 of 5 files at r3, 7 of 7 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[degasus]

I haven't reviewed everything in detail, but what I've seen looks very good. LGTM

[psennermann]

For me RealXFB with Vulkan still doesn't work