VideoBackends: Internal resolution frame dumping

[stenzek]

This is sort of a RFC if such a feature would be worth doing for the other backends. It was really straightforward with Vulkan as the various final blit paths are all separated, but for the others it'd require a bit more refactoring.

---

This change is

[RisingFog]

I feel that this would be a nice feature to have for people like me who do a lot of video dumping in Dolphin.

[lioncash]

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

{
  // Use the resolution of the EFB as a guide for determining the frame dump size.
  TargetRectangle target_rect;

This code segment should probably be its own function.

---

Comments from Reviewable

[BhaaLseN]

If this stays Vulkan (or: !all backends) only, I'd like to have the Checkbox disabled for the other backends (that is, an according BackendInfo.bSupportsXxxx). Otherwise I like the idea ;)

[stenzek]

@BhaaLseN it's not a lot of work to port it to the other backends, just means rendering the frame dump to a FBO instead of to the window, and reading that back. I'll implement it on GL first, then maybe D3D.

[CrossVR]

PR #4173 and PR #3880 are relevant for the other backends.

[stenzek]

This PR can be considered complete now. It's only implemented for OpenGL and Vulkan, however the check box will not show up anymore if a user is using D3D. I wanted to keep this PR as small as possible, so if someone wishes to add support for the other backends it's not super-difficult, just a fair bit of code shuffling.

[MayImilae]

₍₍ (ง ˘ω˘ )ว ⁾⁾

Let's get this reviewed so we can merge it before the progress report! This is awesome!

[stenzek]

I should also probably mention this fixes a regression introduced with the frame dumping changes where an extra frame would be "buffered" after saving a screenshot, so the image actually written to the disk would be one "screenshot" behind when the user actually triggered it.

[BhaaLseN]

Can't really comment on the API-specific parts, but the cleanup of UpdateDrawRectangle/CalculateTargetSize (always being called with the same static fields at all caller sites) looks like a good idea (assuming it is very unlikely that we ever need other values in there diverging from the current static ones).

---

Reviewed 1 of 10 files at r2, 11 of 15 files at r5.
Review status: 12 of 19 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoCommon/RenderBase.cpp, line 450 at r5 (raw file):

}

float Renderer::CalculateDrawAspectRatio(int target_width, int target_height)

I hope we never get a target_height or s_backbuffer_height equal to zero in here...

---

Comments from Reviewable

[stenzek]

Review status: 12 of 19 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoBackends/OGL/Render.cpp, line 1637 at r4 (raw file):

Previously, degasus (Markus Wick) wrote…

FlushFrameDump();

Moved to SwapImpl().

---

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

Previously, lioncash (Mat M.) wrote…

This code segment should probably be its own function.

Done as CalculateFrameDumpDrawRectangle().

---

Source/Core/VideoCommon/RenderBase.cpp, line 450 at r5 (raw file):

Previously, BhaaLseN (BhaaL) wrote…

I hope we never get a target_height or s_backbuffer_height equal to zero in here...

Don't see how that could happen. target_width comes from FramebufferManager, which IIRC uses a "max(width, 1)" statement for the size, since creating zero-sized textures is invalid. Same with the backbuffer dimensions, this comes directly from the swap chain, and it shouldn't be zero for the same reason.

That said, it might be worth adding an assert just in case though?

---

Comments from Reviewable

[degasus]

Small nitpicks, but LGTM

---

Reviewed 2 of 10 files at r2, 2 of 4 files at r4, 10 of 15 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoBackends/OGL/Render.cpp, line 1737 at r5 (raw file):

  glGenTextures(1, &m_frame_dump_render_texture);
  glActiveTexture(GL_TEXTURE10);

Why 10? We usually use 9 for all kind of temporary usages. Or is 9 already used within the swapping logic?

---

Source/Core/VideoCommon/RenderBase.cpp, line 513 at r5 (raw file):

  rc.bottom = static_cast<int>(std::ceil(draw_height));

  // Ensure divisibility by 4 to make it compatible with all the video encoders

heh, which video encoder is not able to process it? We don't guarantee a multiple of 4 in the other way either.

---

Comments from Reviewable

[stenzek]

Review status: 8 of 18 files reviewed at latest revision, 3 unresolved discussions.

---

Source/Core/VideoBackends/OGL/Render.cpp, line 1737 at r5 (raw file):

Previously, degasus (Markus Wick) wrote…

Why 10? We usually use 9 for all kind of temporary usages. Or is 9 already used within the swapping logic?

GL_TEXTURE9 is used as the source image unit for the final blit to screen, but it doesn't matter since it's only bound here for the allocation/parameter setting, so I've changed it to 9 also.

---

Source/Core/VideoCommon/RenderBase.cpp, line 513 at r5 (raw file):

Previously, degasus (Markus Wick) wrote…

heh, which video encoder is not able to process it? We don't guarantee a multiple of 4 in the other way either.

Not sure, this comment was left in the original code. If it's not an issue with libav, maybe it was from when we were using VFW previously? If libav has no such restriction on formats I'll drop it.

---

Comments from Reviewable

[degasus]

Reviewed 10 of 11 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Source/Core/VideoCommon/RenderBase.cpp, line 513 at r5 (raw file):

Previously, stenzek (Stenzek) wrote…

Not sure, this comment was left in the original code. If it's not an issue with libav, maybe it was from when we were using VFW previously? If libav has no such restriction on formats I'll drop it.

libav doesn't require this AFAIK. I don't remeber such a code either.

---

Comments from Reviewable

[degasus]

---

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable