Conversation
b1acd91 to
12d626d
Compare
12d626d to
a4e72df
Compare
a4e72df to
97639de
Compare
97639de to
281a0da
Compare
281a0da to
b16e6b6
Compare
3Shain
left a comment
There was a problem hiding this comment.
No need to make change for every comment. Mainly it's the design of IMTLDXGIMonitor /IMTLDXGIFactory that needs refactoring.
| struct MTLDXGI_MONITOR_DATA; | ||
| namespace dxmt { | ||
| class MTLDXGIMonitor; | ||
| }; | ||
|
|
||
| DEFINE_COM_INTERFACE("8f804acf-f020-4914-98d4-a951da684b7f", IMTLDXGIMonitor) | ||
| : public IUnknown { | ||
| virtual HRESULT STDMETHODCALLTYPE SetMonitorData(const MTLDXGI_MONITOR_DATA *pData) = 0; | ||
| virtual HRESULT STDMETHODCALLTYPE GetMonitorData(MTLDXGI_MONITOR_DATA **ppData) = 0; | ||
| }; | ||
|
|
||
| DEFINE_COM_INTERFACE("413dae46-5707-46e1-b66d-6e58b9f15113", IMTLDXGIFactory) | ||
| : public IDXGIFactory6 { | ||
| virtual dxmt::MTLDXGIMonitor * STDMETHODCALLTYPE GetMonitor() = 0; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I'm strongly against introducing new COM interfaces and I've been always working on removing existing one. The only valid use case is when I want to export DXMT functionality for 3rd party users, which doesn't apply here.
There was a problem hiding this comment.
Honestly the whole concept of monitor is unnecessary. How is it different from output?
| void | ||
| Presenter::updateGammaLUT() { | ||
| if (gamma_curve_ && gamma_curve_->updated) { | ||
| gamma_curve_->updated = false; |
There was a problem hiding this comment.
shouldn't here a pso invalidation happen? pso_valid.clear();
and I think the whole function should be a part of Presenter::synchronizeLayerProperties() because it also effectively queries information for presenting the next frame and rebuild pso when something changed.
There was a problem hiding this comment.
at later changes I was thinking about putting into synchronizeLayerProperties too, but wasn't sure if it's right thing. I'll look into
| monitor_info_ = factory_->GetMonitor(); | ||
| MTLDXGI_MONITOR_DATA monitorData; | ||
| for (uint32_t i = 0; i < DXMT_DXGI_GAMMA_CP_COUNT; i++) { | ||
| float pos = GetGammaControlPointPosition(i); | ||
| monitorData.gammaCurve.Red[i] = monitorData.gammaCurve.Green[i] = monitorData.gammaCurve.Blue[i] = pos; | ||
| } | ||
| monitorData.gammaCurve.gammaIsIdentity = true; | ||
| monitorData.gammaCurve.updated = false; | ||
| monitor_info_->SetMonitorData(&monitorData); |
There was a problem hiding this comment.
So the "source of truth" for gamma curve is from IDXGIFactory/Monitor instead of IDXGIOutput? Shouldn't the IDXGISwapChain pull gamma curve from IDXGIOutput for every Present()? That means gamma curve should not be passed to constructor of Presenter but as a parameter (const reference/pointer) of Presenter::synchronizeLayerProperties() and Presenter holds its own copy of MTLDXGI_MONITOR_DATA for change detection.
There was a problem hiding this comment.
Com<IDXGIOutput1> target_; to get corresponding IDXGIOutput of swapchain in fullscreen state (where gamma correction makes sense). And it can be NULL in windowed state => no gamma correction. Actually I found another oversight (not in this PR), that in SetFullscreenState() the second parameter of target output can be NULL: SetFullscreenState(TRUE, NULL) is valid, but DXGI will choose the output based on the swap-chain's device and the output window's placement. per MSDN.
There was a problem hiding this comment.
I realized you may need to get access to some DXMT-internal methods from IDXGIOutput (i.e. get instance of MTLDXGIOutput), then you can check MTLD3D11Fence/MTLD3D11FenceImpl for reference and make appropriate modifications.
There was a problem hiding this comment.
So the "source of truth" for gamma curve is from
IDXGIFactory/Monitorinstead ofIDXGIOutput? Shouldn't theIDXGISwapChainpull gamma curve fromIDXGIOutputfor everyPresent()? That means gamma curve should not be passed to constructor ofPresenterbut as a parameter (const reference/pointer) ofPresenter::synchronizeLayerProperties()andPresenterholds its own copy ofMTLDXGI_MONITOR_DATAfor change detection.
concept was a bit influenced from DXVK, due connection between MTLDXGIOutput and IDXGISwapChain. there are multiple instances MTLDXGIOutput, like from MTLDXGIAdatper::EnumOutputs. so its not reliable place to store gamma curve and fetch from IDXGISwapChain I didn't saw how I can pass gamma curve to IDXGISwapChain.
There was a problem hiding this comment.
So the "source of truth" for gamma curve is from
IDXGIFactory/Monitorinstead ofIDXGIOutput? Shouldn't theIDXGISwapChainpull gamma curve fromIDXGIOutputfor everyPresent()? That means gamma curve should not be passed to constructor ofPresenterbut as a parameter (const reference/pointer) ofPresenter::synchronizeLayerProperties()andPresenterholds its own copy ofMTLDXGI_MONITOR_DATAfor change detection.
I cannot fetch from IDXGIOutput, I need to pass through one instance like IDXGIFactory.
| ? WMTGetPrimaryDisplayId() | ||
| : WMTGetSecondaryDisplayId(), | ||
| &native_desc_); | ||
| monitor_info_ = factory_->GetMonitor(); |
There was a problem hiding this comment.
Why GetMonitor() here but QueryInterface() in other place?
|
Also please split your commit into several smaller chunks. |
yes, that was a plan, I wanted to see feedback from you first. |
I'm not sure what to do. I need to pass gamma curve from |
What's the problem to access the target output in swapchain in fullscreen state? Do you mean it's possible to get multiple instances of |
what I mean The factory was convenient as it's created along with device. I'll check if using adapter would be fine. For reference: 3 x Output |
Let's check the Windows behavior. I think there is at least one misalignment because I often get page fault when replying DXMT trace in RenderDoc on Windows 🤔. That worths a separated PR. |
I did api trace under windows, it shows 2 adapters and 1 factory. When I run api trace with DXMT: So, last output could store gamma curve. But how to trigger update in swap chain from this output🤔 |
|
Per MSDN:
I understand it as |
|
another instance from api trace log return some error, and get dropped.
I did check how entering and leaving fullscreen involve Output with DXMT. I also checked what happened with instances on Windows. It's hard to figure out what is going on from api traces. Like to check if IDXGIOutput instances are connected, or old ones are invalided (my guess is yes). |
D3D11 gamma support for game
Titanfall 2andQuantum Break