Fix MSAA FBO and fix MSAA MRT FBO #3601
Conversation
…g its own + it is ofFbo's responsibility to unbind, not the renderer's + follows the example of the fixed function renderer
…ed through allocate(texData) would get a different TEXTURE_TARGET as requested, if not using the OPENGLES renderer + seems like something left over from a refactor
…ending on any ofTexture allocation side-effects.
…ave to do round-trips to the GPU with glGetInteger, which can stall the pipeline and are super slow on some cards.
+ render buffers are only marked dirty if they have been rendered to + mutlisampling renderbuffer are blitted out to textures at the corresponding binding point upon getTexture + blitting only happens when the texture has changed and was marked dirty
|
tested on osx 10.9.5 but with gl 4.0 (highest i have available) and it looks the same as yours.
|
|
hey @tgfrerer, when i roll back to master, the fbo renders correctly. |
OpenGL ES might have set a default frame buffer for MSAA rendering to the window, bypassing ofFbo, so we can't trust ofFbo to have correctly tracked the bind state. Therefore, we are forced to use the slower glGetInteger() method to be sure to get the correct default framebuffer.
|
Thanks for testing, Kyle, Lukasz! @julapy, after spending some time (and £79 on an iOS dev membership :/ ) I seem to have found the issue: in iOS with MSAA enabled in the windowing system, instead of going straight to the screen render buffer, a default framebuffer is bound, which is not known to ofFbo since it bypasses the openFrameworks pipeline at generation. Therefore, ofFbo can't track the bind state of GL_FRAMEBUFFER_BINDING correctly, without using these costly As a fix, i've added an
Is the function, where in both renderers, GLES1, and GLES2 the default framebuffer is set up. It could make sense to, from this method, let ofFbo know about the current default buffer, maybe by initialising the framebuffer through ofFbo, but i wonder if that's possible right now. I've pushed a fix to the PR, tested on iOS/iPhone 5c, with GLES1 and GLES2. |
|
hey Tim, sorry that you got sucked into the ios membership scam :) |
|
It is a scam, isn't it? At least i know now in part where these record quarterly profits came from. Great! i've just tested it on linux, ubuntu 14.10, with the latest binary nvidia driver / dwm (unity is broken for GLFW), and it works perfectly. I say if @julapy's tests come back ok, we should be ready to merge. |
|
yep, looks good on iOS too! |
|
thanks, brilliant! i'd say we're ready to merge =) ping @arturoc |
tgfrerer
changed the title
|
looks correct to me except that with the multiwindow branch i also removed most global state ans static variables to be able to use OF GL objects from different threads. this introduces a static stack for the framebuffers which reintroduces some kind of global state. is it necessary to fix this problem? if not i would just remove it by now and if it is try to move it to the renderers instead |
|
Makes perfect sense to add it to the renderers! Even better, iOS would know about it's current renderer/fbo binding state and wouldn't have to deal with |
+ also adds a distiction between renderer.fbo.bind and renderer.fbo.begin: before, bind and begin would be used almost interchangeably, this way, an fbo can be bound through a renderer without setting up modelview matrices, which is closer to the GL intent. + since fbo.bind changes the state of the renderer, such parts are executed within the context of the renderer, which adds the ability to extend these methods in the future with more renderer-specific code.
|
The latest changes implement fbo binding state as a member variable of their respective GL renderers, like @arturoc suggested.
Tested on windows, programmable + fixed function GL, with the above test project, ofFboTrails example, and another project, which gave me this result (best viewed on a big screen):
|
| void ofGLProgrammableRenderer::unbind(const ofFbo & fbo){ | ||
| glBindFramebuffer(GL_FRAMEBUFFER, 0); | ||
| void ofGLProgrammableRenderer::end(const ofFbo & fbo){ | ||
| fbo.unbind(); |
There was a problem hiding this comment.
this should now be unbind(fbo) to avoid global calls when calling through the renderer
There was a problem hiding this comment.
hmm, i need the indirection to mark the buffers dirty which have been rendered to, if using MRT.
There was a problem hiding this comment.
mmh ok, let me take a look later. in general calling bind/unbind begin/end or draw in GL objects is now the only operations that are thread unsafe so to work with threads you should be able to do:
renderer.bind(fbo)
and that should be safe but if that internally calls fbo.bind and fbo calls ofGetGLRenderer then it'll not be safe any more.
why do we need the frameBuffer stack? does that solve some problem? i've been also trying to avoid keeping this kind of state even inside the renderer since it's really prone to errors. it would be weird to have nested fbo binds no?
There was a problem hiding this comment.
yes, nested fbos seem weird to me too. We have an issue open about them #1742. Would be nice to do without, but i felt i needed them to reliably keep track of fbo state.
There was a problem hiding this comment.
Hey @arturoc, can you explain a little more what the thread-issue is?
I've got a bit of time to fix things if neccessary, but it's a bit of a pain keeping this mergeable. And since MRT is broken since (or shortly before) multiwindow it would be nice to have it working again for 0.9.
I see that multithreading was discussed in #3260, but i'm not sure how current the discussion there is.
There was a problem hiding this comment.
So if i understand correctly, you are talking about thread-safety and a race-condition around accessing ofGetCurrentRenderer() on the CPU (openFrameworks) side, and not on the GPU/OpenGL side?
I think i could find a way to address that.
Just to make sure: we are not talking about the implications of multiple contexts, each on its own thread, sharing resources, hopefully. Because this would be where things would get really, really complex, and i'm not sure oF can handle it 100% currently without introducing sync objects, mutexes and the like =)
Also, it promises to be super slow (http://blog.gvnott.com/some-usefull-facts-about-multipul-opengl-contexts/) because of possible per-command context swaps.
In such a scenario it appears FBOs would not be shareable, because they are Container objects, like VAOs (https://www.opengl.org/wiki/OpenGL_Object#Container_objects). But how even to document this - or flag this in code? Or make an ofFbo not shareable?
It's really hard to find good documentation on this, i trawled the specs and the OpenGL registry with very meagre results. The most promising thing i could find was this: http://www.seas.upenn.edu/~pcozzi/OpenGLInsights/OpenGLInsights-AsynchronousBufferTransfers.pdf and it didn't appear too enthusiastic.
Sorry if i seem a bit confused, but i've been rewriting, and banging my head against this for 4 hours now, without finding a solid solution, and i'm a bit scared about the maintenance implications of ofGLFWWindowSettings.shareContextWith = mainWindow; to be honest =)
There was a problem hiding this comment.
yeah, only talking about thread safety in the cpu, particularly about avoiding singletons (right now only the current window which contains a current renderer). not even about the renderer itself being thread safe, each thread should create it's own renderer
everything in the GL side is just responsibility of the user.
shareContextWith allows 2 windows usually in the same thread to share resources like textures or buffers but it's something that GLFW itself allows and again responsibility of the user to use correctly.
you could even create 2 windows in different threads that share no GL objects at all but they can work without crashing by not accessing any singletons
sorry if it wasn't clear before
There was a problem hiding this comment.
In such a scenario it appears FBOs would not be shareable, because they are Container objects, like VAOs (https://www.opengl.org/wiki/OpenGL_Object#Container_objects).
also this is why now every window contains it's own renderer, some resources can't be shared so the programmable renderer wouldn't work if there was only one renderer for all the windows as it was till now, the shaders won't work across different contexts even if they are shared.
but this is not something we should care about for all the objects, if someone wants to use openGL in such an advanced scenario i think it's safe to suppose that they'll know which objects can be shared or not
There was a problem hiding this comment.
Thanks so much for clearing this up- it makes all sense now =)
I'll see to send a fix tomorrow.
Btw would it be a good idea to somewhere formalise or externalise the design guidelines for the API and the renderer that were developed over time?
I think it could be a helpful reminder for us and anyone wanting to send PRs...
I could start whacking something up into an issue or the GitHub wiki with the label "design strategy" which we could link to from the Contribute readme.
Some general principles around:
- which states the renderer tracks and which not
- where the renderer's responsibility ends
- how GL state will be restored in core if in doubt
- gl best practices for the renderers
There was a problem hiding this comment.
by the way, yes a document specifying some design guidelines would be
great to have!
On Mi, 2015-02-18 at 14:30 -0800, Tim Gfrerer wrote:
In libs/openFrameworks/gl/ofGLProgrammableRenderer.cpp:
@@ -1315,12 +1321,46 @@ void ofGLProgrammableRenderer::bind(const ofFbo & fbo, bool setupPerspective){
}//----------------------------------------------------------
-void ofGLProgrammableRenderer::unbind(const ofFbo & fbo){
- glBindFramebuffer(GL_FRAMEBUFFER, 0);
+void ofGLProgrammableRenderer::end(const ofFbo & fbo){- fbo.unbind();
Thanks so much for clearing this up- it makes all sense now =)
I'll see to send a fix tomorrow.
Btw would it be a good idea to somewhere formalise or externalise the
design guidelines for the API and the renderer that were developed
over time?I think it could be a helpful reminder for us and anyone wanting to
send PRs...I could start whacking something up into an issue or the GitHub wiki
with the label "design strategy" which we could link to from the
Contribute readme.Some general principles around:
* which states the renderer tracks and which not * where the renderer's responsibility ends * how GL state will be restored in core if in doubt * gl best practices for the renderers—
Reply to this email directly or view it on GitHub.
|
related #1863 |
…o fix_msaa_and_mrt_msaa_framebuffers
…rer/openFrameworks into fix_msaa_and_mrt_msaa_framebuffers Conflicts: libs/openFrameworks/gl/ofGLRenderer.cpp
fixes merge conflict.
…ramebuffer id remove framebuffer binding stack * since fbos keep track of the binding state before they were bound this should work. add a member variable to ofFbo to track previous framebuffer binding, and add getters and setters. use fbo's previous framebuffer binding variable to keep track of framebuffer binding state within the renderer bind/unbind fbo from within renderer by using the renderer's explicit methods remove unnecessary roundtrip to getGLRenderer (an FBO can safely assume to be called from a GL renderer) make ofFbo::updateTexture public, + add documentation to bind/unbind methods, and to updateTexture methods. +use glGetInteger where it promises to be tricky but not performance-critical. (adds comment to explain) cleanup, add some more comments
|
The latest updates do not round-trip to fetch the current renderer upon bind() / unbind().
tested on:
|
…fers Fix MSAA FBO and fix MSAA MRT FBO
|
yay! :) |
|
[:beers:] |
This PR is a fix for #3598, and it also adds the ability to use mutlisampling (MSAA) with fbos that have multiple render targets (MRT) -- this used to be an issue in the past and was also left over with a big TODO in the inline comments of
ofFbo=PI got a bit carried away fixing the initial issue, but figured it would be nice to be able to use more than one MSAA buffer with
ofFbo.@julapy, would you mind checking out if this fixes #3598 - also, maybe, if everything still works as expected on iOS?
I've split this PR up into more or less self-contained commit units, each with some more information in the commit message. Also, I've added some inline comments where it seemed to make sense.
I'm too tired now to test on OS X and Linux, and with the fixed function renderer (and about to head out to get a very late Friday 🍺), but Windows/VS2012 programmable works a breeze. If interested parties could give it a spin, i can see to fix anything that pops up on Monday when I'm back at work.
Test Project
Here is a one-file project as a gist, which I used to test this against:
https://gist.github.com/tgfrerer/94a1e0443a8a62866019
Test Render
I'm rendering the same scene into 3 FBOs with each 3 render targets,
left has 4xMSAA, middle 0, right 16xMSAA. Top are render targets 0, middle render targets 1, bottom render targets 2. For best results, look at the image with 1:1 magnification =)