Cleanup of framebuffer ping-pong in PostProcessing::render

[sbilliet]

I encountered this code while working on the "speed streaks" effect, and I figured it's best to create a separate PR for this.

Why

Cleanup of the framebuffer ping-pong in PostProcessing::render(). There is no visible behaviour change: i.e. the framebuffer returned by render() still returns the same image as before. The main issue with the code was that adding effects in the chain was prone to accidentally using the wrong input/output buffer pair or producing a black screen.

Changes

• Make the ping/pong between (in_fbo/out_fbo) buffers explicit by adopting std::swap whenever an effect writes into out_fbo (i.e. any effect that ping/pongs) and adding documentation for the cases where we explicitly don't need to swap because we're additively blending to the input buffer directly.

• I also explicitly bind the correct target for renderLightning, as it was otherwise reliant on the effect before it doing so. (i.e. if someone were to do a Blit in between it, which unbinds the FBOs, the effect would not render properly)

• Last thing that I changed was changing the MLAA arguments to make their usage more explicit and ensure they follow the same rules as the rest of the PostProcessing effects (as described).

Agreement

By creating a pull request in stk-code, you hereby agree to dual-license your contribution as
GNU General Public License version 3 or any later version and
Mozilla Public License version 2 or any later version.

This includes your previous contribution(s) under the same name of contributor.

You also confirm that this contribution is your own original work
and that it does not contain code generated by AI tools.

Keep the above statement in the pull request comment for agreement.

[Alayan-stk-2]

I've looked at the code.

There are several nice things in there: the extra comment explaining how different effects use the buffers, the comments marking explicitly blend-in operations, the explicit bind for weather, and generally making the execution state easier to follow which is particularly nice considering that incorrect assumptions about the program's execution state are a massive source of bugs.

However, I did some searching, and what I've found seems to indicate that ping-pong isn't the right terminology here. To quote from the clearest explanation I found:

A ping pong buffer is a technique used in computing and programming to manage data in a way that allows concurrent processing and data handling without conflicts. Essentially, it involves two memory buffers that exchange roles between reading and writing tasks to optimize the workflow. When one buffer is being read, the other can be written to, enabling smooth and efficient data handling.

By using a ping pong buffer, you can significantly enhance the efficiency of your data processing. While one buffer is being read, the other can be written simultaneously. This means that data processing can continue without interruption, reducing latency and increasing throughput. You'll find this especially beneficial in applications requiring real-time data processing.

Unless I'm missing something, using the ping-pong terminology here could be misleading?

[sbilliet]

Hello Alayan, thanks for the review.

I think I understand where you're coming from, it's good to be clear about the intent of the code.

The term ping-pong rendering or buffering is often used for these kinds of patterns in graphics programming.

The definition you have linked does describe what's happening here correctly, but does not specifically frame it in relation to graphics programming.

When we have a shader that needs to read the frame buffer, perform some operation on that data, and then update the framebuffer with the changes. Because the GPU works on different threads in parallel, we can not safely bind the same texture to be both used for reads and writes concurrently, without dealing with memory coherency problems (this is the conflicts that's being referred to in the definition you've linked).

The second part of the definition you have linked touches on efficiency, in the context of the GPU, one could technically use the same resource to read and write to as long as you're ok with using synchronization primitives (i.e. interruptions). This could be between the different threads executing the current shader (atomics, MemoryBarriers) or between different shaders/steps being executed concurrently using ResourceBarriers (or other) (i.e. different shaders can be overlapped, so we also need to guarantee safety).

Sorry for elaborating too much in case this might already be familiar to you, sometimes good to rehash and write this up for myself and potentially others too.

While it was tricky to find one specific definition in regards to rendering, I can link you with some findings that use the term in context: Link1, Link2, Link3 etc.

That being said, I can add a comment to the code or change the terminology to be in and out buffer, if it's preferred?

Kind regards,
sbilliet

[Alayan-stk-2]

Sorry for elaborating too much in case this might already be familiar to you, sometimes good to rehash and write this up for myself and potentially others too.

That's fine. Graphics programming is not where my expertise lies, and it's better to overexplain slightly than to underexplain.

While it was tricky to find one specific definition in regards to rendering, I can link you with some findings that use the term in context: Link1, Link2, Link3 etc.

I think the clarity issue that I see is that the "ping-pong" term gives an impression of symmetry. It's not merely that some outputs become inputs for a next phase (that's incredibly common in any kind of software if you think about it), but about both framebuffers being fully equivalent. That's what I also get from this source you gave:

Now, without looking at the documentation, I'd assume the smart efficient way to swap framebuffers when you call the swap operation is not to move all their contents, but rather to simply swap where each reference points.

If that's correct, then the under-the-hood workings would indeed be ping-pong, because a given region in memory would alternatively serve both roles.

But the code sits one level of abstraction higher, where the in buffer variable consistently points to the source buffer and the out variable serves as target or temp buffer when the effect can't be done in-place.

So, the way I see it:

• For the main function, ping pong is just mentioned in comments (the buffers are still named in and out) and it's arguably fine once you better understand what is going on, but without some extra context it can be confusing.
• For the MLAA function, the ping-pong names highlight that neither framebuffer has a special role, however it doesn't really help to understand how they are used. The issue mostly existed before the rename too, the comments are lacking.

So what I'd suggest is adding/expanding comments somewhat to be clearer to someone who doesn't already have strong familiarity with graphics programming.

[sbilliet]

Now, without looking at the documentation, I'd assume the smart efficient way to swap framebuffers when you call the swap operation is not to move all their contents, but rather to simply swap where each reference points.
If that's correct, then the under-the-hood workings would indeed be ping-pong, because a given region in memory would alternatively serve both roles.

This is correct, and if you take a look at the code that's exactly what the std::swap achieves. The pointer just gets swapped around, no large memory copies/transfers needed. Simple re-interpretation of what becomes the source(read) and what becomes the destination(write).

But the code sits one level of abstraction higher, where the in buffer variable consistently points to the source buffer and the out variable serves as target or temp buffer when the effect can't be done in-place.

Exactly, semantically using "in" and "out" makes sense to differentiate what will be passed as parameters and what will be bound as the rendertarget. The technique of ping-ponging the buffers, serves two points specifically:

• one is that it's the most efficient way to pass the data around.
• two is that's it's just clean (just set up the renderpass and once you're done call std::swap), no need to go look at what the renderpasses before did and ensure you bind it's output buffer as your input and it's input buffer as your output etc.

For the MLAA function, the ping-pong names highlight that neither framebuffer has a special role, however it doesn't really help to understand how they are used. The issue mostly existed before the rename too, the comments are lacking.

Yes indeed, and I will extend this PR by adding some descriptive comments to the code that was there before the PR.

As a general note, I believe we both agree and it's merely discussing to get a better understanding of what ping-pong means as well as making the code easier to understand for other contributers correct? :)

Cheers,
sbilliet

[Alayan-stk-2]

As a general note, I believe we both agree and it's merely discussing to get a better understanding of what ping-pong means as well as making the code easier to understand for other contributers correct? :)

Yes, I think we essentially agree. It would be for other contributors, or for me in a year or two... It's not always easy to understand your own code after some time when the reasoning behind it has been forgotten, let alone someone else's.

[sbilliet]

Hello Alayan, I've just updated the PR.

Added a section to the beginning of PostProcessing::render() to try and summarize the ping-pong technique.

Additionally went through the MLAA pass(es) and annotated the individual passes, also changed the naming of the variables to more closely align with what purpose they serve.

In general thanks for the review and the discussion on the matter, let me know if I should make further changes :)

Have a good one,
Kind regards,
sbilliet

[Alayan-stk-2]

You forgot to update the names in the .hpp file, but that was trivial to fix myself.

Everything looks good, so I'm merging.

[sbilliet]

Thanks Alayan! 👍