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

Move a significant amount of video backend logic to VideoCommon #7753

Open
wants to merge 4 commits into
base: master
from

Conversation

9 participants
@stenzek
Copy link
Contributor

stenzek commented Jan 28, 2019

Huge scary pull request ahead!

This pull request moves a decent amount of logic from the video backends to VideoCommon. It's something I've been putting off for a while now, but finally found the motivation/time to do.

In addition to the maintainability improvements, as all backends are now considered equal, it adds things like:

  • Post-processing to D3D
  • GPU texture decoding to D3D
  • Vulkan-style EFB cache to D3D and OpenGL
  • Vulkan-style EFB poke batching to D3D and OpenGL

Some things makes easier in the future:

  • Better state tracking in OpenGL -> performance improvements (since all state is now controlled via pipelines, we know what it is, and it hasn't been changed by random functions)
  • Post-processing of XFB copies, rather than just the scanout (something I started a long time ago but was put off because of how much duplication there was in backends)
  • Multi-threading of the backends
  • Who doesn't like fewer lines of code for the same functionality
  • Re-implementing D3D12? :) (if we can keep it to 5k lines of code or so, in line with the other backends)

Unfortunately, due to it being backend-specific, it does remove the stencil buffer bounding box emulation. I figured now that we have Vulkan/Metal on macOS, this removes one of the main uses for it.

I'm sorry for the size of this PR, but with most of the changes touching the same code, if I separated it, I'd be writing a lot of code just to throw it out again. I started doing that, but it got old quick :(

A rough work list:

  • Use abstract textures for EFB emulation
  • Move FramebufferManager to common and create implementation using abstract classes
  • Move TextureCache functionality to common (EFB copies, palette texture conversion)
  • Remove a lot of duplicated logic in Vulkan (largest backend by far...)
  • Re-implement GPU texture decoding
  • Implement texel buffers on OpenGL and Vulkan (for palette texture conversion)

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 2 times, most recently from eccf568 to 7241237 Jan 28, 2019

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Jan 28, 2019

Fixes Brawl/Panel De Pon on D3D. https://bugs.dolphin-emu.org/issues/6572
Fixes Offset Bloom in Monster Hunter Tri.

NES VC Games do not render at all.
Twilight Princess is kinda broken
Mario Golf is...
image
Mario Superstar Baseball has weird texturing defects
image
New Super Mario Bros. Wii Main Menu
image

@stenzek stenzek added the WIP label Jan 28, 2019

@BhaaLseN
Copy link
Member

BhaaLseN left a comment

Love the diff delta so far!

Do those Initialize methods that set up static/global/whatever unique_ptrs have any reason to release/.reset them early in case initialization fails? Because right now, they do not; but as far as I can tell from the diff they didn't really either before...but I'm wondering if theres anything hanging around there that should probably be released before the next one tries to use things.

if (!g_ActiveConfig.backend_info.bSupportsOversizedViewports)
{
x = std::min(x, static_cast<float>(g_renderer->GetCurrentFramebuffer()->GetWidth() - 1));
y = std::min(y, static_cast<float>(g_renderer->GetCurrentFramebuffer()->GetHeight() - 1));

This comment has been minimized.

@BhaaLseN

BhaaLseN Jan 28, 2019

Member

Looks a bit strange that both are clamped downwards in position and then limited in size (which in the most extreme case is a one by one viewport somewhere in a corner?) rather than...i dunno, scaling/translating things or something else.
Does anything ever hit this case anyways?


// Clear the renderable textures out.
g_renderer->SetAndClearFramebuffer(m_efb_framebuffer.get(), {0.0f, 0.0f, 0.0f, 0.0f}, 0.0f);
// g_renderer->SetViewport(0.0f, 0.0f, static_cast<float>(GetEFBWidth()),

This comment has been minimized.

@BhaaLseN

BhaaLseN Jan 28, 2019

Member

Does this commented out code have any significance, or can it be removed?


bool FramebufferManager::CompileConversionPipelines()
{
// TODO: This really needs an enum..

This comment has been minimized.

@BhaaLseN

BhaaLseN Jan 28, 2019

Member

...so make it one? ;)


bool FramebufferManager::PopulateDepthReadbackTexture()
{
// StateTracker::GetInstance()->OnCPUEFBAccess();

This comment has been minimized.

@BhaaLseN

BhaaLseN Jan 28, 2019

Member

Commented out code?

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 3 times, most recently from 49da8c2 to 94d57be Jan 29, 2019

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Jan 30, 2019

Fixes the D3D crash in Paper Mario.

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 3 times, most recently from 782d720 to 5bdc91d Jan 30, 2019

@Helios747

This comment has been minimized.

Copy link
Contributor

Helios747 commented Jan 30, 2019

I'd be cool with reimplementing D3D12 if it can be proven that maintaining it (read, fixing issues) can be done by fixing common code, so that we're not trapped to only graphics devs that have windows 10 set up and can be bothered.

Intel iGPU users would rejoice.

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Jan 31, 2019

Isn't the true solution to add a new maintainable D3D7 backend if we're worrying about people with bad GPUs?

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Jan 31, 2019

Isn't the true solution to add a new maintainable D3D7 backend

thanks, i hate it

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Jan 31, 2019

I'd be cool with reimplementing D3D12 if it can be proven that maintaining it (read, fixing issues) can be done by fixing common code, so that we're not trapped to only graphics devs that have windows 10 set up and can be bothered.

The idea is that with all the driving logic in common, it shouldn't need much in the way of actual maintenance, since it's just converting to the API. The backend code shouldn't ever need to change, unless it's for new features :)

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 9 times, most recently from 00d4381 to b25d261 Jan 31, 2019

@stenzek stenzek added needs testing and removed WIP labels Feb 3, 2019

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 2 times, most recently from eaa046a to dfc051b Feb 3, 2019

@M220

This comment has been minimized.

Copy link

M220 commented Feb 3, 2019

I Can't launch any game with OpenGL in android. And the log that's located in dolphin-emu -> Logs, is empty for some reason. Because it's not an specific game, you can probably reproduce this bug yourself but if you want the errors anyway, then here is the errors in order :

  1. failed to compile ps shader : ~~/Dump/bad_ps_0000.txt
    1:16:S0012: Global variable initializer must be a constant expression
  2. falied to compile post-processing shader
  3. (error 1 but with bad_ps_0001.txt)
  4. failed to initialize renderer classes
  5. fifo shutting down while active
  6. failed to initialize video backend

Vulkan and software are both working and I had seen no problem with them so it't just an OpenGL Specific bug.

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch from 1c56082 to eb3c857 Feb 14, 2019

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 14, 2019

@iwubcode I did find an issue where a stitched texture could be transitioned inside a render pass, which shouldn't happen now. I doubt it's what's causing the crash, but you never know. Speaking of which, is it a game crash, or a dolphin/gpu crash? JMC47 was unable to reproduce it with a NV GPU.

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Feb 15, 2019

@iwubcode I did find an issue where a stitched texture could be transitioned inside a render pass, which shouldn't happen now. I doubt it's what's causing the crash, but you never know. Speaking of which, is it a game crash, or a dolphin/gpu crash? JMC47 was unable to reproduce it with a NV GPU.

Thanks @stenzek . I tried the new build but still see the same issue.

Dolphin itself doesn't crash, the image freezes up and then turns to black (no sound of course). Even hitting the 'stop' button (multiple times) does not stop the rendering and I must force-quit the application.

EDIT: Just for historical purposes. It seems like the issue has something to do with XFBs. Turning on 'Immediate Mode' or unchecking 'XFB to Texture Only' fixes the issue

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch from eb3c857 to 4375edd Feb 15, 2019

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 15, 2019

@iwubcode Again, not sure if it's the same issue, but we found strided XFB copy reconstruction could issue out-of-range copies. This should really crash/reset the driver, but it might be related.

I'd be curious what the call stack looks like when it's frozen if you attach a debugger to dolphin and pause it (the CPU/GPU threads).

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Feb 16, 2019

@stenzek - the new build didn't help solve the issue unfortunately.

Here's the callstack you requested (slightly trimmed, let me know if you want more):

DolphinD.exe!Common::Semaphore::Wait() Line 25
DolphinD.exe!Vulkan::CommandBufferManager::PrepareToSubmitCommandBuffer() Line 200
DolphinD.exe!Vulkan::Renderer::ExecuteCommandBuffer(bool submit_off_thread, bool wait_for_completion) Line 403
DolphinD.exe!Vulkan::VertexManager::ResetBuffer(unsigned int vertex_stride) Line 155
DolphinD.exe!VertexManagerBase::UploadUtilityVertices(const void * vertices, unsigned int vertex_stride, unsigned int num_vertices, const unsigned short * indices, unsigned int num_indices, unsigned int * out_base_vertex, unsigned int * out_base_index) Line 280
DolphinD.exe!Renderer::DrawImGui() Line 1082
DolphinD.exe!Renderer::Swap(unsigned int xfbAddr, unsigned int fbWidth, unsigned int fbStride, unsigned int fbHeight, const MathUtil::Rectangle<int> & rc, unsigned __int64 ticks) Line 1222
@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 17, 2019

@iwubcode huh, odd. Does turning off backend multi-threading in advanced options change anything?

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Feb 17, 2019

@iwubcode huh, odd. Does turning off backend multi-threading in advanced options change anything?

No it does not

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 17, 2019

Could you check the callstack with the MT option off? I'm curious where it's getting stuck, it seems like it's somewhere in presentation.

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Feb 17, 2019

@stenzek - seems to be the same callstack as before. So it's getting stuck in presentation when waiting on the semaphore for submitting the command buffer?

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 17, 2019

With multithreading off, the semaphore shouldn't ever block. I'll double-check the code.

stenzek added some commits Jan 29, 2019

TextureCache: Bind textures/samplers after loading all textures
Since loading textures can result in rendering, e.g. partial copies, we
don't want to disturb partially-bound GX state.
TextureConverterShader: Write EFB2Tex XFB copies with alpha value of 1
This way we don't end up with artifacts of the EFB's alpha values in
frame dumps. XFB copies loaded from RAM also set the alpha to 1, so this
will match.
TextureCache: Don't copy out-of-range rectangles when stitching textures
This can cause driver crashes or GPU hangs if we do.

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 2 times, most recently from c3f8b89 to d4d0f74 Feb 17, 2019

@iwubcode

This comment has been minimized.

Copy link
Contributor

iwubcode commented Feb 17, 2019

Confirmed fixed. Here was the original issue:

The vertex buffer is full after the semaphore is drained, so it tries to submit the current buffer (and in the process, tries to drain the already-drained semaphore)

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch 6 times, most recently from d73db61 to 322be67 Feb 17, 2019

@falikhukoma1996

This comment has been minimized.

Copy link

falikhukoma1996 commented Feb 19, 2019

I want to run the game but it can't be opened for this version still force close.

i'm playing wii game fatal frame 2 & 4.

@JMC47

This comment has been minimized.

Copy link
Contributor

JMC47 commented Feb 19, 2019

Works fine here. Are you sure it runs fine in master too?

@M220

This comment has been minimized.

Copy link

M220 commented Feb 19, 2019

Maybe He's Talking about android🤔, because any game that I tried to open in android ( with renderer set to OpenGL ) ended up closing itself (not emulator) but with vulkan or software, everything is like before and I can load any game that I have and it runs without a problem

@weihuoya

This comment has been minimized.

Copy link
Contributor

weihuoya commented Feb 19, 2019

crashed at Renderer::ClearScreen when call glClearDepthf on android.

@stenzek stenzek force-pushed the stenzek:videocommon-all-the-things branch from 322be67 to f039149 Feb 19, 2019

@stenzek

This comment has been minimized.

Copy link
Contributor Author

stenzek commented Feb 19, 2019

Reproduced the crash, except it was in SetAndClearFramebuffer (glClearDepth instead of glClearDepthf). Fixed as of latest push.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment