GBM/X11/Wayland: Implement asynchronous texture uploads #25139

sarbes · 2024-05-05T22:53:05Z

Description

This PR implements asynchronous texture uploads, by having a helper EGL context.

Motivation and context

Previously, the texture uploads of assets (fanart, thumbs, etc.) were synchronous with the UI rendering, despite most of the texture pipeline (reading, decoding, converting, scaling) being asynchronous.

This meant that the main thread would need to wait for the upload to complete, which would block execution. While I don't have concrete numbers, the workload of processing a typical 1920x1080 texture (memcopy + driver stuff) could tax a low-end device for quite some time, which normally lead to dropped frames.

While there is no guarantee that async uploads will behave better, there is much more room for the driver to perform well. I expect Mesa to perform better.

The feature is currently gated behind an advanced setting:

<advancedsettings>
  <gui>
    <asynctextureupload>true</asynctextureupload>
  </gui>
</advancedsettings>

The async upload could be further refined by uploading the texture in chunks, with pauses in between. In testing, pauses between texture processing steps reduced dips in framerate, presumably by relieving stress on the memory bus.

How has this been tested?

Runs fine on X11 with GL and GLES.
A prototype ran fine on GBM on my Ordroid C2, but needs retesting.
Wayland is fully untested.

What is the effect on users?

Less jank, better system performance.

Screenshots (if appropriate):

Estuary with synchronous (UI texture) upload disabled, only asynchronously uploaded textures are being shown (text is still synchronous):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
Student submission (PR was done for educational purposes and will be treated as such)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

neo1973 · 2024-05-05T23:45:26Z

Looks promising 🙂

I gave it a quick test on Wayland and GBM (GL + GLES) and noticed issue with thumbnail creation:

Many errors like these in the log:

2024-05-06 01:25:08.909 T:68440   debug <general>: Caching image '/home/mark/.kodi/userdata/addon_data/plugin.video.themoviedb.helper/crop_v2/cropped-9124a98b31c97388baf04fc948ae27a4.png' to '1/1ea8ff83.png':
2024-05-06 01:25:08.909 T:68440   debug <general>: cached image 'special://masterprofile/Thumbnails/1/1ea8ff83.png' size 719x298
2024-05-06 01:25:08.909 T:68440   error <general>: ffmpeg[0x515001176d00]: [swscaler] bad src image pointers
2024-05-06 01:25:08.909 T:68440   error <general>: SWS_SCALE failed for thumbnail: special://masterprofile/Thumbnails/1/1ea8ff83.png
2024-05-06 01:25:08.909 T:68440   error <general>: Failed to CreateThumbnailFromSurface for special://masterprofile/Thumbnails/1/1ea8ff83.png

Without asynctextureupload all thumbs are created and no errors are in the log.

Full log: https://paste.kodi.tv/akuficugab.kodi

lrusak

I'm not really sure what is async about this as it effectively blocks the current thread until the upload is done? So it really only works if the upload is happening from another thread? Do you know how many texture uploads happen in other threads? (I'm on my phone so I'm being lazy and asking other than checking myself).

The switching of the eglcontexts could be problematic if synchronization isn't handled correctly.

I wonder if it would be better to have a shared EGL context that is bound in another thread that basically just waits for uploads to enter a queue. This would avoid the need for locking and switching contexts.

xbmc/guilib/Texture.cpp

lrusak · 2024-05-05T23:47:35Z

xbmc/guilib/TextureGL.cpp

@@ -229,6 +229,11 @@ void CGLTexture::LoadToGPU()
 m_loadedToGPU = true;
 }

+void CGLTexture::SyncGPU()
+{
+ glFinish();


This effectively blocks no?

Yes, that's desired. It blocks the upload thread, and waits until the texture is visible for the GPU.

xbmc/utils/EGLUtils.cpp

lrusak · 2024-05-05T23:54:21Z

xbmc/utils/EGLUtils.cpp

+ return false;
+ }
+
+ m_textureUploadLock.lock();


I don't love the fact of locking in the middle of these functions. I assume because the texture uploads may happen from other threads? It would be nice if there was the ability to add texture uploads to a queue to avoid needing the lock.

I think there can be multiple jobs in parallel. Only one can have the upload context.

lrusak · 2024-05-05T23:56:25Z

xbmc/windowing/WinSystem.h

+ * \brief Binds a shared context to the current thread, in order to upload textures asynchronously.
+ * \return Return true if a texture upload context exists and the binding succeeds.
+ */
+ virtual bool BindTextureUploadContext() { return false; }


It would be nice to avoid polluting CWinSystemBase with more stuff. Can this be moved somewhere else? Possibly the caller can cast the window system to the EGL intermediate class?

I've experimented with that, but I couldn't come up with an universal solution which does not look hacky. If you could give me some pointers, I'd appreciate it.

See here:

xbmc/xbmc/cores/VideoPlayer/VideoRenderers/HwDecRender/RendererDRMPRIMEGLES.cpp

Line 59 in 64e0ecf

auto winSystemEGL =

The problem is that x11 uses a different EGL context class so that might be annoying. It was always the intention to get x11 to use the same EGL class that wayland and gbm are using, it just hasn't happened yet.

Yeah, that's why I shelved the branch for a long time. I'm still running X11. I'll see what I can do.

On the flip side, I believe that GLX and DX11 can have a very similar implementations. It is not EGL specific, if that's your concern.

xbmc/windowing/X11/GLContextEGL.h

lrusak · 2024-05-06T00:02:33Z

xbmc/utils/EGLUtils.cpp

+ return false;
+ }
+
+ bool success = eglMakeCurrent(m_eglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);


Should this not bind back to the other context?

The job has no context to begin with.

Wouldn't this effectively unbind the main context if called from the main thread? I don't think we bind the main thread context unless windowing is being inited

No, it checks if there is a context bound, and bails if so.

And in its current state, it is only called in the threaded texture loader.

sarbes · 2024-05-06T02:14:39Z

To give some context how (I think) the current system works:

XBT textures are done synchronously. They are loaded from disk and uploaded to the GPU in the main rendering thread, blocking execution. This might stall the renderer, but the textures are guaranteed to be available for the next frame.

For assets like fanart or thumbnails, a upload job gets spawned. This job reads, decodes, converts and scales the texture in its own thread; otherwise we could stall for seconds. When the job is done, the main thread picks up the prepared texture and uploads it to the GPU. This stalls the thread.

My PR gives the Jobs a GL/GLES context in which they can execute GL calls. So in addition of doing all the normal texture processing, the job also uploads the texture to the GPU.

sarbes · 2024-05-08T08:27:45Z

Looks promising 🙂

I gave it a quick test on Wayland and GBM (GL + GLES) and noticed issue with thumbnail creation:

For caching, it is expected to keep the intermediate CPU buffer alive. But uploading deletes it, in this case right before the caching. I've moved the call for the async upload to the actual texture processing job, so the texture can be cached before upload.

neo1973 · 2024-05-09T19:32:30Z

Tested the latest version and can confirm that caching images works as expected 🙂

I also think that making the calls to CTexture::LoadToGPUAsync() explicit helps a lot to understand what's happening. I'm not really happy with the "Async" in the function name, because the function itself doesn't do anything async, but I'm not sure if I can come up with something better tbh. Maybe LoadToGPUInUploadCxt() works better? It's a bit of a mouthful but but would describe the intention better.

sarbes · 2024-05-10T12:04:29Z

Maybe LoadToGPUThreadSafe?

neo1973 · 2024-05-10T21:55:12Z

Doesn't sound right to me. Especially because it's not safe to call from the render thread, right?

Anyway, that shouldn't be a blocker, and from my side this looks pretty solid 🙂

sarbes · 2024-05-10T23:00:36Z

It is safe to call from the rendering thread. It checks if there is a context bound, and bails if so.

I don't really like bringing the "context" into the name, because I think it will be different for Vulkan. For DX11, it should be similar tho.

neo1973 · 2024-05-10T23:06:35Z

It is safe to call from the rendering thread. It checks if there is a context bound, and bails if so.

Ahh, that's what the CServiceBroker::GetWinSystem()->HasContext() is doing. Got it 👍

sarbes added 4 commits May 5, 2024 23:46

GL/GLES prepare for async texture uploads

162bf61

X11: Implement async texture uploads

3bb94d0

GBM: Implement async texture uploads

b96997d

Wayland: Implement async texture uploads

8722b68

sarbes requested review from yol and lrusak as code owners May 5, 2024 22:53

sarbes added Component: GL rendering Component: GLES rendering v22 "P" Type: Improvement non-breaking change which improves existing functionality labels May 5, 2024

sarbes added this to the "P" 22.0 Alpha 1 milestone May 5, 2024

lrusak reviewed May 6, 2024

View reviewed changes

sarbes added 3 commits May 8, 2024 10:05

Make upload call explicit

1b4d3ce

Cleanup EGLUtils

b818c0a

Cleanup GLContextEGL

84625af

yol removed their request for review May 10, 2024 09:22

neo1973 approved these changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GBM/X11/Wayland: Implement asynchronous texture uploads #25139

GBM/X11/Wayland: Implement asynchronous texture uploads #25139

sarbes commented May 5, 2024

neo1973 commented May 5, 2024

lrusak left a comment

lrusak May 5, 2024

sarbes May 6, 2024

lrusak May 5, 2024

sarbes May 6, 2024 •

edited

lrusak May 5, 2024

sarbes May 6, 2024

lrusak May 10, 2024

sarbes May 11, 2024 •

edited

lrusak May 6, 2024

sarbes May 6, 2024

lrusak May 10, 2024

sarbes May 11, 2024 •

edited

sarbes commented May 6, 2024

sarbes commented May 8, 2024

neo1973 commented May 9, 2024

sarbes commented May 10, 2024

neo1973 commented May 10, 2024

sarbes commented May 10, 2024

neo1973 commented May 10, 2024

GBM/X11/Wayland: Implement asynchronous texture uploads #25139

Are you sure you want to change the base?

GBM/X11/Wayland: Implement asynchronous texture uploads #25139

Conversation

sarbes commented May 5, 2024

Description

Motivation and context

How has this been tested?

What is the effect on users?

Screenshots (if appropriate):

Types of change

Checklist:

neo1973 commented May 5, 2024

lrusak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarbes May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarbes May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarbes May 11, 2024 • edited

Choose a reason for hiding this comment

sarbes commented May 6, 2024

sarbes commented May 8, 2024

neo1973 commented May 9, 2024

sarbes commented May 10, 2024

neo1973 commented May 10, 2024

sarbes commented May 10, 2024

neo1973 commented May 10, 2024

sarbes May 6, 2024 •

edited

sarbes May 11, 2024 •

edited

sarbes May 11, 2024 •

edited