Page MenuHome

GHOST/DRW: Add interface to manage offscreen opengl contexts
AbandonedPublic

Authored by Clément Foucault (fclem) on Feb 10 2018, 3:34 PM.

Details

Summary

This is an attempt to create a separate context for the drawmanager.

This has two benefits:

  • We can run this context in parallel with the UI context. (multithreaded F12 render)
  • The Batches' VAOs are creating under this only context making multiple window viewport work (because every viewport rendering uses this context).

All resources are shared with all contexts (as always).

For now only Linux and Windows are supported.
(Windows support is courtesy of @Germano Cavalcante (mano-wii))

There is still crashes and instabilities due to the old way of managing contexts and FBO / VAO. Freeing is particulary bad.
This does not fix VAOs used under each UI windows contexts (i.e. the normal widget [see sky texture node]).

To reviewers: I want to know if this approach is not too much invasive in respect to the WM code. And if the GHOST part makes sense. Also not sure about all the naming of functions.

Diff Detail

Repository
rB Blender
Branch
temp-drawcontext
Build Status
Buildable 1229
Build 1229: arc lint + arc unit

Event Timeline

Clément Foucault (fclem) retitled this revision from GHOST: Add interface to manage offscreen opengl contexts. to GHOST/DRW: Add interface to manage offscreen opengl contexts.Feb 10 2018, 3:51 PM
Clément Foucault (fclem) edited the summary of this revision. (Show Details)

Overall this make a lot of sense to me.

Would the eventual goal be to do all drawing into offscreen framebuffers? If each area or region has its own framebuffer, we could get rid of the triple buffer textures and give the windows just an RGB buffer without any Z/A/multisample storage. It's something I wanted to try at some point, and it would solve issues like VAOs for the normal widget drawing.

intern/ghost/intern/GHOST_SystemWin32.cpp
315

It would be good to have a comment explaining this, it's not obvious to me why an offscreen context would be tied to a window. Is it an API limitation? Maybe it determines with GPU is used if there are multiple GPUs used for different monitors?

It also seems problematic for background mode OpenGL render, but I'm not sure if this code would be reused for that.

323

Indentation seems too deep here.

331

I guess this false will need to be replaced by some variables?

source/blender/draw/intern/draw_manager.c
380

Only write locks are used for this read-write mutex, so unless you are planning to use read locks you could use a regular mutex instead.

3641–3644

Maybe add a DRW_ogl_select_ctx_enable() and use it here? Just to keep that locking logic together a bit.

4307

I would prefer to have the full name DRW_opengl_context_create() here for clarity, it's not so long and abbreviations are not always obvious.

source/blender/windowmanager/intern/wm_window.c
100

Any reason we can't use wm->winactive here?

If it's to avoid passing context, you can use wmWindowManager *wm = G.main->wm.first;. If it has a different meaning, that seems confusing and the name/comment should be more clear on what the difference is.

2068

Maybe use WM_opengl_context_create(), it's not so obvious otherwise.

2070

And this one could also be GHOST_CreateOpenGLContext, so it's a consistent name throughout.

For Windows API check with @Germano Cavalcante (mano-wii)

For my part I agree with the comments in general but ...

source/blender/windowmanager/intern/wm_window.c
100

I used it because winactive was not set. But in fact I should have used windrawable. Is it ok to use wm.first? can there be multiple window manager? can there be none?

2070

I find the naming of GHOST_CreateOpenGLContext pretty redundant for GHOST and the context could be tied to the window (not in the api for now). This is to differentiate them. The offscreen context has no surface attached to it. And maybe we will have a background context creation in the future GHOST_CreateBackgroundContext. Since it's GHOST the context is obviously OGL, right?.

source/blender/windowmanager/intern/wm_window.c
100

It's safe to use wm.first, there is always exactly one window manager, and the same code is used in other places.

2070

I don't mind it being called GHOST_CreateOffscreenContext. I just found it a bit confusing that the same thing was called three slightly different names in three different modules. Maybe it can be "offscreen context" everywhere then. It's just a small nitpick though.

  • DRW Opengl Context: Adress comments on D3057
  • Merge branch 'blender2.8' of git@git.blender.org:blender.git into temp-drawcontext
  • GHOST: Style, add comments, and make sure that the system has at least one window
  • Create an offscreen context with own hDC
  • GHOST: offscreen context support for macOS.
  • GHOST: Remove exception from multiple context on intel cards
  • Eevee: Fix SSR in multiviewport.
  • DRW: Fix DRW_draw_select_loop and DRW_draw_depth_loop
  • Merge branch 'blender2.8' of git@git.blender.org:blender.git into temp-drawcontext
  • Merge remote-tracking branch 'origin/blender2.8' into temp-drawcontext
  • DRW: Use new gawain context.
  • Merge remote-tracking branch 'origin/blender2.8' into temp-drawcontext
  • Eevee: Fix SSR not appearing in render.
  • Fix Edit Mode selection triggering an OpenGL error.
  • DRW: Fix selection & cursor autodist not working after rendering.
  • Merge remote-tracking branch 'origin/blender2.8' into temp-drawcontext
  • Merge remote-tracking branch 'origin/blender2.8' into temp-drawcontext
Brecht Van Lommel (brecht) requested changes to this revision.Feb 22 2018, 3:50 PM
Brecht Van Lommel (brecht) added inline comments.
intern/ghost/intern/GHOST_SystemWin32.cpp
315

Solving this will require passing GHOST_GLSettings to GHOST_CreateOpenGLContext(). Not sure how much you are relying on OpenGL debugging, but seems like keeping that could be somewhat important.

source/blender/draw/intern/draw_manager.c
381

Rename to g_ogl_context_lock or something like that?

source/blender/editors/render/render_opengl.c
850

Maybe oglrender->fx should be created and destroyed in the context as well? It seems safer to just do it for all the OpenGL data structures anyway, even if currently they can be shared.

source/blender/editors/space_view3d/view3d_draw.c
2140

Disable the context here.

source/blender/gpu/intern/gpu_viewport.c
530–535

DRW_opengl_context_disable() should be after the glEnable / glDisable I think? At least GPU_viewport_bind() doesn't seem to do any OpenGL state changes before calling DRW_opengl_context_enable().

source/blender/windowmanager/intern/wm_init_exit.c
212

DRW_opengl_context_destroy() doesn't seem to be called anywhere.

source/blender/windowmanager/intern/wm_window.c
1047–1056

It seems more clear to me if the immediate mode and gawain calls that are here go to the DRW_opengl_context functions, Now it's kind of spread out.

Also not sure if this TODO is important to fix still.

This revision now requires changes to proceed.Feb 22 2018, 3:50 PM
source/blender/windowmanager/intern/wm_window.c
1047–1056

Actually, wm_window_make_drawable does the same stuff so I guess it's ok. Just didn't expect that stuff in the WM.

  • GWN: Fix immediate mode when closing a window.
  • Merge remote-tracking branch 'origin/blender2.8' into temp-drawcontext

I've committed fixes for macOS in rB813f1c603f64: Fixes for offscreen drawing context on macOS..

Mainly I needed to add a glFlush() call. I don't know if this is just a macOS quirk or what, but I've put it behind #ifdef __APPLE__ in case it has a performance impact on other platforms.

Things seem to be fully working now on macOS + Intel Iris. Some of my comments from D3057#72681 should still be addressed though.