petschau / WinFellow

WinFellow
https://petschau.github.io/WinFellow/
GNU General Public License v2.0
88 stars 13 forks source link

RetroPlatform - screenshot function saves black/partial screen area #20

Closed carfesh closed 6 years ago

carfesh commented 6 years ago

The screenshot function will currently snapshot a completely black screen area (D3D mode) or only the visible parts of the screen (DirectDraw). Saving a screenshot to the pictures folder repeatedly from Amiga Forever 2017 will crash the emulator.

Examine why fullscreen screenshots no longer work, this already exists in 0.5.6.

Since the clipping editor uses the screenshot function internally, this also currently fails.

carfesh commented 6 years ago

A few observations:

In DirectDraw mode the issue occurs when starting the clipping editor right away; it disappears when doing things modifying the screenmode, like switching display scale. This may be an initialization issue. Messages sent from the host to WinFellow look OK and identical to the working case.

In Direct3D mode the screen is captured completely black regardless of screenmode changes.

carfesh commented 6 years ago

Thanks to git bisect I could narrow down that Direct3D screenshots are broken since commit a3ab8d8aae55b83413d58330834bd466d770b464. Sadly it contains a lot of changes that I'll have to examine further.

DirectDraw screenshots are much harder to narrow down, as there seems to be not one singular commit breaking the functionality; at one point it works, at another it doesn't but there's also commits in between where it does not work, but the build contains other issues and the error manifests differently. I also found an occurrence where it was broken, worked again and later was broken again - at that time development was concentrating around D3D and the issues weren't noticed during testing.

carfesh commented 6 years ago

The following error is logged to the debug log when graphics debugging is active and a screenshot is being taken via D3D:

D3D11 ERROR: ID3D11DeviceContext::CopyResource: Cannot invoke CopyResource when the source and destination are not the same Resource type, nor have equivalent dimensions. Src has the following dimensions: Width (640), Height (400), Depth (1), MipLevels (1), and ArraySize (1). Dst has the following dimensions: Width (1280), Height (800), Depth (1), MipLevels (1), and ArraySize (1). [ RESOURCE_MANIPULATION ERROR #284: COPYRESOURCE_INVALIDSOURCE]

Apparently the current rectangle calculations don't take the scaling factor into account. This is also why this was not noticed during testing- the D3D screenshots work fine when scaling factor is set to 1x from the beginning. When just adjusting for that, D3D and DirectDraw screenshots both have the same "black areas" issue; D3D still must support switching displayscale factor on the fly.

petschau commented 6 years ago

Do you also have problems with filtered screenshots in d3d11 or is it just my laptop? I get this log:

DXGI ERROR: IDXGISurface1::GetDC: GetDC can only be called for textures that were created with the D3D10_RESOURCE_MISC_GDI_COMPATIBLE flag. [ MISCELLANEOUS ERROR #89: ] First-chance exception at 0x76F308B2 in WinFellow.exe: Microsoft C++ exception: _com_error at memory location 0x107CF8A4.

Strangely it asks for the flag, I would have expected D3D11_RESOURCE_MISC_GDI_COMPATIBLE.

carfesh commented 6 years ago

Odd; I don't experience that message. I've tried both on my development PC (Win10, nVidia GeForce GTX 970 with most recent driver), as well as in a Hyper-V Win10 VM. What OS and what kind of graphics card do you use for your laptop?

The screen saved to the screenshots folder was always black for me until my recent commit 3b6526acbd02786530f720ac57b73e5eae5fcb86.

With that one, the screenshot texture area is adjusted to always match the Amiga screen buffer and now screenshots are being taken again in D3D, also in 2x, 3x and 4x. It still has an issue in that it initially will not use the whole raw area in the raw screenshot, but use the filtered one instead. That disappears when switching scaling factor, so that's probably some missing initalization somewhere. So it's still bugged, but it is a huge improvement. I did not test DirectDraw with that yet, and also not the standalone builds.

carfesh commented 6 years ago

You had that issue in standalone WinFellow, didn't you? With your recent commit it starts working for me as well. I had only tested in Amiga Forever before, standalone WinFellow had failed for me as well. I'll still have to look at why the screenshot size/buffer is initially wrong, and re-test DirectDraw screenshots. But looking at the clock I think that's enough progress for today - have a nice evening! 👍

petschau commented 6 years ago

Yes, it is standalone mode. I found a reference to the exact same issue here:

https://www.gamedev.net/forums/topic/656408-direct3d-11-creating-gdi-compatible-texture-for-backbuffer/

Seems to work fine for me as well. Nice when bug fixes are that easy :-)

carfesh commented 6 years ago

The code isn't the most complex, but there's just so many scenarios and special cases to test for where the screenshot code is being used; it's always been error prone in the past. The different graphics drivers don't make this easier..

Once I have worked out the other issues, I'll try and come up with a step-by-step regression test case description. I guess it makes sense to have a that so that for future changes we can catch this more easily before we publish a release.

carfesh commented 6 years ago

Turns out the missing/black screen areas were caused by initialization of the internal clipping area for the Amiga host screen buffer. The code applying the configuration did apply the output clipping area to the internal one, which I suppose is intended to happen for a standalone WinFellow session. I've removed the duplicate call to it, it is already being set up in the RetroPlatform initialization code and with that change both D3D as well as DirectDraw screenshots seem fine now.

Will leave this open for the time being until I have finished all tests related to this; I've also added a new regression test case description targeted specifically at the recent issues.

carfesh commented 6 years ago

Testing were alright, closing this.