bgfx::CallbackI::screenshot callback never gets called on MacOS Metal

[georged-youi]

requesting a screenshot with bgfx::requestScreenshot, does not trigger a screenshot callback on MacOS Metal.

Looking at the code, the issue seems to be with m_saveScreenshot, which is set to true only in requestScreenShotPre, method that is used nowhere.

Forcing a reset with BGFX_RESET_CAPTURE, triggers the screenshot callback, because m_capture is not NULL anymore, but requestScreenshot should work without BGFX_RESET_CAPTURE.

[attilaz]

requestScreenshot can be used to get the content of a window's backbuffer.
On Metal this is CAMetalLayer's nextdrawable's texture. By default it has a property of framebufferOnly. This means we cannot get data from it. (We cannot even copy it to other texture like other renderers do) https://developer.apple.com/documentation/metal/mtltexture/1515749-framebufferonly?language=objc

Possible solutions:

• We can override this behaviour, but the docs suggest it will have a performance penalty. I haven't benchmarked how severe it is. But always paying the extra cost is not the best solution. I think we can enable this only for frames where it is needed, but we should know about the requestScreenshot request at the beginning of the frame to set it.
  https://developer.apple.com/documentation/quartzcore/cametallayer/1478168-framebufferonly?language=objc

• Another solution to render an ordinary rendertarget texture that has similar properties to back buffer, get data from this texture and copy content with submitQuad to backbuffer. (This is implemented right now, but missing msaa and multiple window support)
  This also needs info at the before renderer::submit to handle rendering to the temp buffer instead of backbuffer.

So both method has an extra performance penalty for rendering. We should only pay for this extra cost for frames where it is needed.

To test this feature I think it would be beneficial to add a request screenshot feature to 22-windows as it is the only example that has multiple windows.

@bkaradzic

[bkaradzic]

@attilaz Yes, we can change CommandBuffer::RequestScreenShot command to be pre-submit command (for all renderers), which whould just cache values, and then it would get executed after rendering. This way you would be able to switch Metal into mode to capture screenshot.

[georged-youi]

also, another thing that I've noticed:

because requestScreenshot hadn't worked, I tried readTexture. This one worked only on macs with intel only gpus. Using a mac with hybrid gpus (intel with shared memory and amd with dedicated memory), readTexture returned only empty buffers.

[attilaz]

@bkaradzic "cache values" means storing every requestScreenShot param pairs (fb handle, filename) in an array in every renderer then process at the end of submit? This seems too much change inside every other renderers.
How about requestScreenShot() generating two commands, for example adding a pre-submit requestScreenShotPrepare(fb). Other renderers could just ignore this.

@georged-youi
I don't have access to mac with dedicated gpu, so I won't be able to check that. Does metal validation give any error?
Maybe we are missing synchronization.
https://developer.apple.com/documentation/metal/mtlblitcommandencoder/1400757-synchronizetexture?language=objc

Can you try replacing renderer_mtl.mm's readTexture with this? https://gist.github.com/attilaz/3cabb555457949099ac611895fb7ee90

[georged-youi]

@attilaz, yes, the new code allows me to successfully read textures on macs with dedicated gpus.

[bkaradzic]

@attilaz

This seems too much change inside every other renderers.

I think it should be only 1 request per frame buffer. And way how it works right now it would generate multiple requests. I'll make change to all renderers and then you can fix Metal.

[attilaz]

@bkaradzic great, thanks!

@georged-youi great! I will test it on my setup, and if I find no problem will send PR.

[Arcnor]

Is there anything missing to be able to fix this? From what I understand from the above, adding a "pre" frame operation will allow us to detect if we're going to request a screenshot or not, then do whatever magic Metal needs to be able to read from its texture, but not sure if there is anything else that requires changing.

[Arcnor]

@bkaradzic @attilaz can any of you offer any guidance on how to fix this? I don't mind opening a PR for it but I need to know where to start, I really need to get this working reliably, screenshots are not optional for my project.

Thanks again.

[klob]

Lastest commit bgfx::CallbackI::screenshot callback gets called, but the data is empty. See example-07-callback

[attilaz]

I have just tested the latest bgfx version on macos (metal renderer) and the 'screenShot' callback function was called with proper data and tga/png images was succesfully saved.
@klob Are you seeing this with metal renderer? Other parameters are missing too, or only the data is null?

[piermorel]

Hi, in relation with this discussion I'm trying to subvert the capture() functions to do what I want, but I ran in the problem that enabling capture (using BGFX_RESET_CAPTURE or requesting a screenshot) breaks the "windows" example (I copied in it the callback class from the "callback" example).

What happens is that the content of all windows gets drawn in the main window (that is captured), and that the secondary windows becomes black. I've tracked it to what happens in setFrameBuffer() when m_screenshotTarget exists, and noticed that requestScreenShot() doesn't use the FrameBufferHandle argument like in other renderers but I'm a bit stuck.

Any ideas on this ?