Skip to content
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

Overlay: Add Direct3D 11 support #1082

Merged
merged 1 commit into from Jan 10, 2014

Conversation

@Kissaki
Copy link
Member

commented Dec 1, 2013

  • Based on an initial patch by Benjamin Jemlich
  • Effects11 code based on changes by nyet

Cleaned up, direct3d 11 related changes only from the previous pull request #177.

This PR adds overlay support for Direct3D 11.
This has been verified in Crysis 2 (main menu), Saints Row 3, Civilization 5 (Demo).
Additionally, as verified with a self compiled SimpleSample.exe from the DirectX SDK using Direct3D 9 - the direct3d 9 overlay now displays in more cases as well.

It adds the fx11 (Effects 11) library as a submodule dependency.
A qmake project file has been added to allow compilation of it as a dependency. On building the mumble_ol.dll (overlay) it is added as a static library, thus no additional dll is necessary.

A build has been tested to not break stuff on Windows 7 x64 (pre-SP) with

  • Crysis 2 (11)
  • Blacklight: Retribution (11)
  • D3D examples (SimpleSample.exe for 9, 10, 11)
  • Smite
  • Guild Wars 1
  • Guild Wars 2
  • Dota 2
  • Awesomenauts
  • Wolfenstein: Enemy Territory (OpenGL)
  • Team Fortress 2 (menu)
  • Battlefield: Bad Company 2
  • League of Legends (menu)

I did not review nyetwurks d3d11 state block and drawing code (yet).
But it works.

.gitmodules Outdated
@@ -13,3 +13,6 @@
[submodule "sbcelt-src"]
path = sbcelt-src
url = git://github.com/mumble-voip/sbcelt.git
[submodule "dependencies/fx11-src"]
path = dependencies/fx11-src

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Having this in a dependencies subfolder is inconsistent with how we handle opus etc.. Any special reason for using a new directory for it while not for the rest?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

I would like to introduce a folder for all of them, as our main folder is getting quite cluttered with a lot of dependencies (src + build folders).

I specifically chose to only put fx11 into a subdirectory in this PR to not have non-overlay/d3d11 changes in it.
I would prefer to do the follow-up moving-the-other-dependencies-into-the-subfolder in a separate PR.

SOURCEDIR=$$replace(BUILDDIR,-build,-src)

!exists(../$$SOURCEDIR/Effect.h) {
message("The $$SOURCEDIR directory was not found. Please make sure it contains the fx11 libraries source.")

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

We should keep the messages here consistent amongst the submodule dependencies

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Ok.


INCLUDEPATH *= ../$$SOURCEDIR/Inc ../$$SOURCEDIR/Binary
INCLUDEPATH *= $(DXSDK_DIR)Include
LIBS *= "$$(DXSDK_DIR)/Lib/x86/*"

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Should use LIBDIRS and explicitely list dependencies as LIBS instead.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Solved by removing.


void D11StateBlock::Capture()
{
this->pDeviceContext->RSGetState(&this->pRasterizerState);

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Codestyle: "this->"

@@ -20,17 +22,21 @@ QMAKE_CXXFLAGS_DEBUG -= -MDd
QMAKE_CXXFLAGS_RELEASE *= -MT
QMAKE_CXXFLAGS_DEBUG *= -MTd

INCLUDEPATH *= "$$(DXSDK_DIR)Include"
INCLUDEPATH *= "$(DXSDK_DIR)Include"

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

$$() and $() have specific meanings. See http://qt-project.org/doc/qt-4.8/qmake-advanced-usage.html#variables . We ought to use them consistently.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Shouldn’t it be $( so the environment variable is evaluated on (makefile-induced) compilation rather than qmake?
I changed it specifically because $( seems to make more sense than $$( here.

Is your point that it should be $$( here? Or that if it’s changed here, it should be changed in other pro files as well?

This comment has been minimized.

Copy link
@hacst

hacst Jan 6, 2014

Member

I just commented here because I wondered where the difference was after seeing that change. I agree that $( seems to be preferable and should be used where possible.

@@ -57,6 +59,7 @@ static HRESULT __stdcall myPresent(IDXGISwapChain *pSwapChain, UINT SyncInterval
#endif

presentD3D10(pSwapChain);
presentD3D11(pSwapChain);

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

This should have some comments on what exactly is happening here. I assume one of the GetDevice calls in the presentD3DXX functions will fail? Are there any more explicit ways to differentiate which code path is supposed to be taken?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

DXGI is the common base “framework”, so I guess not.

The entry point here is the myPresent function, which is passed the DXGI swapchain.
I guess we could store a map with swapchain => 10/11 when preparing.
With a singular mapping we would loose the (theoretical?) support for 1 swapchain with both a 10 and 11 device. Not sure if that is even possible or works right now though … Have not tried.

This comment has been minimized.

Copy link
@hacst

hacst Jan 6, 2014

Member

The primary problem I see is that we call the functions regardless of whether the prerequisites for using their functionality are met. This is also where the DX10.1 bug occurred. The OOP nature of the COM interface allowed the GetDevice call succeed (10.1 devices derive from 10) but the needed DX10 preparation failed earlier as 10.1 uses other DLLs. I think having some more basic checks there might prevent us trouble in the future.

hhAddRef.inject();

Mutex m;
D11State *ds = devices[pDevice];

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Inconsistent map use compared to chains. Not buggy though as far as I can tell.

LONG res = oAddRef(pDevice);
hhAddRef.inject();

Mutex m;

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

We should probably rename that class to ScopedCriticalSection to make it apparent what it's actually doing.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Noted, but would prefer to do it separate from this PR.

Mutex m;
D11State *ds = devices[pDevice];
if (ds)
if (res < static_cast<ULONG>(ds->lHighMark / 2)) {

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Obviously same logic as in D3D10 but I can't say I understand the reason for doing it that way. Needs documentation.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Seems like a fancy optimization, working with probability.

The logic in words:
If more than half the refcounts have been released subsequently, without adding a refcount in between, stop our overlay (because we are likely shutting down).

Actually, the release function is the only place we (can) clean this up. So maybe: In case the application does not adequately release the device but no longer uses it, to still free and clean our stuff we clean up earlier than at 0 (when the device is freed).
If presentation continues, it is recreated in the present function anyway.

Maybe analyse/test this after the PR?
Maybe we can go for a “correct” implementation, freeing on 0. I am also not sure if a device no longer in use by an application would still get present() calls?

ds = new D11State(pSwapChain, pDevice);
chains[pSwapChain] = ds;
devices[pDevice] = ds;
ds->init();

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Should check for initialization failure. Compare to fb56112 .



// D3D11 specific logic for the Present function.
extern HRESULT presentD3D11(IDXGISwapChain *pSwapChain) {

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Same as presentD3D10 I'm not sure why we are returning an HRESULT here which isn't checked anyways. Should probably be a bool or void?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

Sure, let’s go void then.


pDevice->CreateBlendState(&blend, &pBlendState);
float bf[4];
pDeviceContext->OMSetBlendState(pBlendState, bf, 0xffffffff);

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

According to http://msdn.microsoft.com/en-us/library/windows/desktop/ff476462(v=vs.85).aspx we can simply pass NULL for the BlendFactors bf. Same in d3d10.cpp


pDeviceContext->OMSetRenderTargets(1, &pRTV, NULL);

D3D11_BLEND_DESC blend;

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

The settings here had me confused a bit. Wikipedia has a really great explanation on it though. http://en.wikipedia.org/w/index.php?title=Alpha_compositing&oldid=580659153#Description gives an example for the "over" operation which is exactly what we are doing here. Might want to link it here. Same for D3D10.cpp

ods("D3D11: pDevice->CreateDeferredContext failure!");

if (FAILED(hr) || !pDeviceContext) {
ods("D3D11: Failed to create DeferredContext (0x%x). Getting ImmediateContext", hr);

This comment has been minimized.

Copy link
@hacst

hacst Dec 7, 2013

Member

Should have comment under which conditions we expect it to fail. As stated in http://msdn.microsoft.com/en-us/library/windows/desktop/ff476505(v=vs.85).aspx if you create a device using D3D11_CREATE_DEVICE_SINGLETHREADED CreateDeferredContext is expected to fail.

void D11State::init() {
HRESULT hr;

dwMyThread = GetCurrentThreadId();

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Unused (only written to) in DX11 and DX10? Seems like only DX9 does use the thread ID for things.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 5, 2014

Author Member

I’ll just remove it then.

1,2,3,
};

bd.Usage = D3D11_USAGE_DEFAULT;

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Maybe we should zero bd again before this to make it unmistakable we are re-using it?

Also we can use D3D11_USAGE_IMMUTABLE (see http://msdn.microsoft.com/en-us/library/windows/desktop/ff476259(v=vs.85).aspx). Not that it matters much....

This comment has been minimized.

Copy link
@Kissaki

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

Seems to work with immutable, so I guess that’s the better way.

}

void D11State::draw() {
clock_t t = clock();

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

The FPS counter stuff should be its own function imho. Maybe we can even add it to Pipe (and while are at it Pipe isn't very descriptive.... OverlayMessagePipe maybe?).

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

It’s the same for d3d10. Both D10State and D11State extend Pipe. Yeah, the logic should be moved there and Pipe should be renamed.

Can we do that after this PR though?

This comment has been minimized.

Copy link
@hacst

hacst Jan 6, 2014

Member

Sure. I suggest to create issues for the refactoring stuff that you don't feel belong to the scope of the PR. Most of it is pretty small and the biggest issue I see is not forgetting to do it later on ;)


dwMyThread = GetCurrentThreadId();

checkMessage(static_cast<unsigned int>(vp.Width), static_cast<unsigned int>(vp.Height));

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Should probably be named be named checkForMessages

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

Would prefer to do after this PR, as general lib function.

// map/unmap to temporarily deny GPU access to the resource pVertexBuffer
D3D11_MAPPED_SUBRESOURCE res;
hr = pDeviceContext->Map(pVertexBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &res);
//TODO no size-checks? Is that correct and safe?

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Sure. Unless someone changes how the vertex buffer is structured (for which there's no reason) why would we have to check?


for (unsigned int r=0;r< uiHeight; ++r) {
unsigned char *sptr = a_ucTexture + r * uiWidth * 4;
unsigned char *dptr = reinterpret_cast<unsigned char *>(pTexels) + r * mappedTex.RowPitch;

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Interesting RowPitch is used here. I wonder whether we have to actually expect the underlying representation to not be contiguous. If not this would be one memcpy. I couldn't really find anything with a quick google.

D3DXVECTOR2 Tex;
};

class D11State: protected Pipe {

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Definitely needs some comments for members. Also this being named D11State and D11StateBlock containing the DX11 context state is kinda confusing though probably okay.

#include "lib.h"
#include "D11StateBlock.h"
#include "overlay11.hex"
#include "d3dx11effect.h"

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

QtCreator doesn't see this include. Would be interesting to figure out what exactly in the pro files prevents qmake from properly exposing the include path if that's the issue. Maybe some variable is set to build time evaluation that should be qmake run-time?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

Works for me here.

Random guesses:

  • Missing submodule checkout
  • fx11-build qmake run
  • My meantime changes

This comment has been minimized.

Copy link
@hacst

hacst Jan 6, 2014

Member

Ok. Maybe some random messup then. Just wanted to mention it as I was wondering what causes it.

dwMyThread = 0;
}

D11State::~D11State() {

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

This doesn't do a complete cleanup. E.g. the D11StateBlock s aren't freed. Is this intended? If so comment why.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

I guess not intended.

ReleaseAllDeviceObjects is just calling ReleaseObjects(), so removed that method.
~D11StateBlock() calls ReleaseObjects(), so the manual call to it seems unnecessary if we free instead.
As D11StateBlock is never freed from what I can see, I guess that was a mistake.
I changed to free without manual call to release.

(Remembering the previous dependency of D11StateBlock to IUnknown, but no release call.)

this->ReleaseObjects();
}

void D11StateBlock::ReleaseObjects()

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Does this really Release everything it needs to? I haven't checked to be honest but unless some of these automatically invalidate other handles (possible) there should be more.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 6, 2014

Author Member

I added release calls for all member pointer references.

this->ReleaseObjects();
}

void D11StateBlock::GetDeviceContext(ID3D11DeviceContext **ppDC)

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Unused as far as I can tell. Also why not return the pointer instead of passing a ** ?

pSRView->Release();

if (pMyStateBlock) {
pMyStateBlock->ReleaseAllDeviceObjects();

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Those two Release functions are identical.

}
}

void D11StateBlock::ReleaseAllDeviceObjects()

This comment has been minimized.

Copy link
@hacst

hacst Dec 8, 2013

Member

Was there the intention of having two different level's of releasing parts of the state block? Otherwise there should be only one release function present and the alias should be removed.

@hacst

This comment has been minimized.

Copy link
Member

commented Dec 8, 2013

This is pretty much on the level of the DX10 code in terms of quality. Which of course means I had a lot to nag but it should work just fine ;) Apart from my specific comments some general things:

  • Lacks comments
  • Doesn't follow a consistent set of coding guidelines (neither new nor old)
  • Should use explicit NULL checks instead of if(pBla) or if (!pBla)
  • Hardly checks or handles errors anywhere. Probably won't fail gracefully.

We should resolve small and high impact stuff now and do the tedious stuff after merge. E.g. documenting the crap out of it is something we can always do later.

TL;DR: Usable and "reasonably" clean already and with a bit of work should be ready to merge. More potential for improvements later on.

typedef ULONG(__stdcall *AddRefType)(ID3D11Device *);
typedef ULONG(__stdcall *ReleaseType)(ID3D11Device *);

#define HMODREF(mod, func) func##Type p##func = (func##Type) GetProcAddress(mod, #func)

This comment has been minimized.

Copy link
@hacst

hacst Dec 9, 2013

Member

unused

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

I pushed the changes as individual commits here; I will have to rebase against current master before pushing anyway.

So, I guess I may push?!?

@hacst

This comment has been minimized.

Copy link
Member

commented Jan 7, 2014

Push to where?

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

Squashed and rebased to make it one commit again.
I went through all the PR comments here. So I’m done for now.
Ready to merge?

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

To test; we should test with both an AMD and NVidia card. Should test that no crash, displays and updates in games, preferably load 2 game instances per game.

SOURCEDIR=$$replace(BUILDDIR,-build,-src)

!exists(../$$SOURCEDIR/Effect.h) {
message("The $$SOURCEDIR/ directory was not found. You need to do one of the following:")

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 7, 2014

Member

This is a minor thing, but the message is misleading since the only thing to do here is to ensure you have up-to-date submodules.

s/You need to do one of the following/Please update your submodules:/

?

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jan 7, 2014

Author Member

I’d like to change it for all of them. Did not want to change it for all others, so adapted this one to the others for this PR.

This comment has been minimized.

Copy link
@mkrautz
D3D11_MAPPED_SUBRESOURCE res;
hr = pDeviceContext->Map(pVertexBuffer, 0, D3D11_MAP_WRITE_DISCARD, 0, &res);

#ifdef DEBUG

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 7, 2014

Member

I'm not a fan of having this ifdef'd DEBUG. That means that I'll mostly never run into it (as I mostly only run release builds...).

However, MSDN says that static_assert is available in MSVC2010. So we could potentially do a global

const VERTEXBUFFER_SIZE = 4 * sizeof(SimpleVertex);  // I guess we can do constant math? Not sure!

and use that in the creation of pVertexBuffer. Then, before the memcpy (or just after the declaration of vertices[]) we could do a static assert:

static_assert(VERTEXBUFFER_SIZE == sizeof(vertices));

To get some compile-time safety instead.

...Or we could leave the run-time check there if the setRect code path isn't too hot.

void hookD3D11(HMODULE hD3D11, bool preonly);

void checkDXGI11Hook(bool preonly) {
static bool bCheckHookActive = false;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 7, 2014

Member

This still smells wrong to me (I'd use Interlocked to ensure atomicity/synchronization), but let's keep it for now, as it's what we use in the other check*Hook() functions.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

Tested running with (old) fraps and Dxtory fps counters, as well as steam overlay where applicable:

  • Civ 5 demo x11, x9, fullscreen
  • Smite borderless
  • GW2 borderless, windowed, fullscreen
  • GW1 windowed & fullscreen
  • World of Tanks crashes (silently)
  • Saints row the third x11 windowed
  • Portal 2
  • Hard Reset: Overlay flimmers - contrary to dxtory and fraps ones
  • Alien Swarm
  • Blacklight: Retribution
  • Crysis 2 x11
  • Bf: BC 2
  • TF2 borderless
  • Dota 2 borderless
  • Planetside 2 fullscreen

Failed to test UT3, Age of Chivalry - I was somehow missing files.

Have an AMD card.

Without dxtory and fraps World of Tanks does not crash.

The Hard Reset issue is present in the current Snapshot as well.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jan 7, 2014

nVidia for me.

With Steam Overlay only.

Civ5 Gods and Kings DX11, DX9, both fullscreen
BioShock Infinite DX11 fullscreen
Metro Last Light DX11 fails (initialization failure, but keeps going and corrupts things)
Deus Ex: Human Revolution - Director's Cut

With Origin overlay + Shadowplay

BF4 x86 DX11, fullscreen.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2014

Timer tested, has an nvidia card.

  • Planetside 2 works
  • Battlefield 3
  • Battlefield 4
    With Dxtory overlay, ShadowPlay Overlay und OBS at the same time
@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2014

Trials Evolution Gold Edition x11 with uplay overlay works as well.

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2014

CyberKnight verified working

  • Planetside 2
  • Metro Last Light
  • Arma III
  • Assasins Creed Black Flag
Overlay: Add Direct3D 11 support
* Based on an initial patch by Benjamin Jemlich
* Effects11 code based on changes by nyet
Kissaki added a commit that referenced this pull request Jan 10, 2014
Merge pull request #1082 from mumble-voip/olay-11
Overlay: Add Direct3D 11 support

@Kissaki Kissaki merged commit 405d6e4 into master Jan 10, 2014

@Kissaki Kissaki deleted the olay-11 branch Jan 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.