Aleksoid1978 / VideoRenderer

Внешний видео-рендерер
GNU General Public License v3.0
997 stars 110 forks source link

Rename the "Auto Display HDR On/Off" setting to "Windows HDR Mode" and expand its choices to allow turning it on only. #106

Closed clsid2 closed 7 months ago

clsid2 commented 7 months ago

Anything wrong with HDR setting patch?

I have also added fix for this crash: https://drdump.com/Problem.aspx?ProblemID=900295 This crash is more common than above crash dump count suggests. I only get proper dumps for own builds that have pdb uploaded along with a specific mpc-hc release.

I am investigating another crash that I suspect is in UpdateStatsInputFmt() based on offset. But got no proper dump yet. I am not sure what could be failing in there, maybe m_srcParams.str can potentially be null?

Also a possible optimization. The work done inside UpdateStatsStatic() could be skipped if stats overlay is disabled. Then just add this function call when enabling stats overlay.

v0lt commented 7 months ago

Anything wrong with HDR setting patch?

None of the developers can check the patch right now. I don't have an HDR display.

I have also added fix for this crash: https://drdump.com/Problem.aspx?ProblemID=900295 This crash is more common than above crash dump count suggests. I only get proper dumps for own builds that have pdb uploaded along with a specific mpc-hc release.

I can't open your link (login required). But in any case, this does not apply to the pull request under discussion. Also, please do not add patches that do not complement the pull request under discussion.

Also a possible optimization. The work done inside UpdateStatsStatic() could be skipped if stats overlay is disabled. Then just add this function call when enabling stats overlay.

UpdateStatsStatic is not called for every frame. Therefore, there will be no noticeable optimization if you disable it.

clsid2 commented 7 months ago

You can login as guest to view the stacktrace.

What the HDR patch does is just give option whether the renderer can turn OFF display HDR mode when it was already ON before loading the renderer and you are playing SDR video. Some people prefer to keep it on and do not like the display switching modes. So existing option corresponds to the On/Off choice. And new option only turns On HDR when needed. Patch should be readable even if you don't have HDR screen.

v0lt commented 7 months ago

Please remove the "Fix crash in Tex_t::Create()" patch from the pull request. I do not know how to do it.

clsid2 commented 7 months ago

Done. Rebased on git master.

clsid2 commented 7 months ago

I think this may fix the other crash that I am tracking:

--- a/Source/VideoProcessor.cpp
+++ b/Source/VideoProcessor.cpp
@@ -145,8 +145,14 @@ void CVideoProcessor::UpdateStatsInputFmt()
    }
    else if (m_iSrcFromGPU == 11) {
        m_strStatsInputFmt.append(L"D3D11_");
+   } else {
+       m_strStatsInputFmt.append(L"unknown");
+       ASSERT(FALSE);
+       return;
+   }
+   if (m_srcParams.str) {
+       m_strStatsInputFmt.append(m_srcParams.str);
    }
-   m_strStatsInputFmt.append(m_srcParams.str);
v0lt commented 7 months ago

I think this may fix the other crash that I am tracking:

I don't understand why this happens, but just in case I set the string "unknown" to CF_NONE (commit 523c313 and commit 1fcfcef). I think this will be enough.

clsid2 commented 7 months ago

Do you guys perhaps have any DVB card or other capture source to reproduce this? Crash issue in CDX11VideoProcessor::MemCopyToTexSrcVideo https://github.com/clsid2/mpc-hc/issues/2424