Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various render optimizations #24

Merged
merged 1 commit into from Aug 12, 2022
Merged

Various render optimizations #24

merged 1 commit into from Aug 12, 2022

Conversation

ori-sky
Copy link
Contributor

@ori-sky ori-sky commented Aug 9, 2022

This patch performs various rendering optimizations, giving a minor performance improvement when using a Waveform source.

As the gradient shader never changes, it does not need to be reloaded and recompiled every render frame, so this patch adds an instance member to cache the compiled shader object. The shader is loaded and compiled before rendering only if it is currently NULL, allowing the compiled shader to be reused in every subsequent frame.

The vertex buffers used for curve and bars rendering are cached in a similar way to avoid recreating and freeing them on every render frame. Additionally, the curve vertex buffer is pre-populated upon creation to avoid needing to update all three components of the vec3 when actually rendering, instead only updating the y component.

@phandasm
Copy link
Owner

Are you sure this is safe?
Last time I tried storing pointers to basically anything related to the rendering context it seemed to cause problems (though it's entirely possible that was due to my unfamiliarity with OBS).

In particular iirc OBS doesn't seem to like freeing vertex buffers outside the rendering thread and will emit warnings for doing so which makes storing them kind of awkward.
Also I don't see the vertex buffers being freed anywhere, as-is doesn't it leak memory whenever update() is called?
vbuf_bars doesn't seem to ever get updated either.

And just as a final note OBS caches compiled shaders internally so I don't think it recompiles every frame unless gs_effect_destroy() actually purges it from the cache (but storing the pointer would still be nice if it's safe to do so).

@ori-sky
Copy link
Contributor Author

ori-sky commented Aug 10, 2022

Thanks for the feedback, you're right, the way it had been done was unsafe and was failing to free resources, as well as generally being an untidy solution.

I've reworked the patch a bit so that it now builds the shader upon instance construction, acquiring the render context in order to do so, as well as rebuilding the vertex buffer in a unified way (again having acquired the render context) in update. Both of these resources are now freed upon instance destruction.

@ori-sky
Copy link
Contributor Author

ori-sky commented Aug 10, 2022

I looked into how OBS does shader caching and it's true that compiled shaders are cached by name, so the performance improvement from this change will be less notable, although this change will still avoid needing to do a lookup (OBS currently stores its cached effects in a linked list so this lookup would be progressively slower the more effects have been cached with OBS).

This patch performs various rendering optimizations, giving a minor
performance improvement when using a Waveform source.

As the gradient shader never changes, it does not need to be reloaded
and recompiled every render frame, so this patch adds an instance member
to cache the compiled shader object. The shader is loaded and compiled
upon instance creation.

The vertex buffers used for curve and bars rendering are cached in a
similar way to avoid recreating and freeing them on every render frame.
Additionally, the curve vertex buffer is pre-populated upon creation to
avoid needing to update all three components of the vec3 when actually
rendering, instead only updating the y component.
@phandasm phandasm changed the base branch from master to develop August 12, 2022 03:13
@phandasm
Copy link
Owner

Got around to testing this and the call to create_vbuf() needs to be moved to the end of update() to avoid crashing when using rounded caps but seems good otherwise so I'll just merge it into a temp branch first.

Thanks for your contribution!

@phandasm phandasm merged commit bf9f73f into phandasm:develop Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants