linux-pipewire: Use dynamic pod builder#10459
linux-pipewire: Use dynamic pod builder#10459GeorgesStavracas wants to merge 1 commit intoobsproject:masterfrom
Conversation
GeorgesStavracas
added
Enhancement
|
@columbarius this should have the same effect as of #8293, mind reviewing this PR? |
The dynamic pod builder implements a dynamically growing buffer for the pods, which can indeed grow quite a lot depending on e.g. the number of modifiers and formats that the GPU advertises. Use that instead of a static 4096 buffer. Build a single pod with each dynamic pod builder, and put the result in a (also dynamic) GPtrArray which holds all pointers to pods.
GeorgesStavracas
force-pushed
the
gbsneto/dynamic-pod-builder
branch
from
fbc22a9 to
76ac74d
Compare
|
For reference, I copied the approach from PipeWire's own code in gstpipewireformat.c. |
| const struct spa_pod **params; | ||
| params = bzalloc(2 * obs_pw_stream->format_info.num * | ||
| sizeof(struct spa_pod *)); | ||
| params = g_ptr_array_new_full(2 * obs_pw_stream->format_info.num * |
There was a problem hiding this comment.
If we want to use a dynamic array we should use the obs DARRAY to keep consistency with the rest of obs.
There was a problem hiding this comment.
DARRAY in this kind of code path (not cold) should be avoided.
There was a problem hiding this comment.
First of all negotiation is cold. And second we know the size so there arnt any performance concerns to begin with. Its just a matter of using the api we all know or using the gptr api.
| struct spa_fraction min_framerate; | ||
| struct spa_fraction framerate; | ||
|
|
||
| spa_pod_dynamic_builder_init(&pod_builder, NULL, 0, 4096); |
There was a problem hiding this comment.
This should be a lot smaller now. instead of 50 formats with 50 modifiers it just needs to hold the 50 modifiers. I assume it grows automatically, but we can just see what the size is on amd and use that I guess.
| struct spa_fraction min_framerate; | ||
| struct spa_fraction framerate; | ||
|
|
||
| spa_pod_dynamic_builder_init(&pod_builder, NULL, 0, 4096); |
There was a problem hiding this comment.
spa_pod_dynamic_builder_clean is referenced in other places. I cant find any docs on spa_pod_dynamic_builder (not sure why i hoped it would exist), but can you confirm that we dont need to clean up after init.
|
Seems to work fine on my amd box, maybe we should also backport this into 30.1 or something since there are some lingering reports of crash on pipewire start which have been related to these buffers being too small before. |
I'm not really a fan of adding more g_ptr and autopointer magic to the pipewire code, since it complicates the code for those not familiar with them (myself included). An alternative approach would be one builder, but recording the offsets of the different pods and then collecting them at the end: columbarius, https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/src/modules/module-filter-chain.c#L1525 |
RytoEX
modified the milestones:
OBS Studio (Next Version),
OBS Studio (Next Major Version)
5 participants
Description
Use the dynamic pod builder, which resizes the pod buffer as needed, instead of a static 4096 buffer which can overflow.
Motivation and Context
The dynamic pod builder implements a dynamically growing buffer for the pods, which can indeed grow quite a lot depending on e.g. the number of modifiers and formats that the GPU advertises.
How Has This Been Tested?
Types of changes
Checklist: