use pipewire buffers #141
use pipewire buffers #141
Conversation
columbarius
force-pushed
the
screencast-pw-driven
branch
from
b323158
to
437d74c
Compare
|
Update: works on my machine! Please take a look if it's going in the right direction. Performance is about doubled (at least from the cpu reading in obs). I have to look into the shutdown procedure, if there is sth. to improve. Sry for the last commit. The changes with using pipewire to allocate buffers, importing them and restructuring the wlroots screencopy handlers were so connected, that i couldn't find a good spot, which didn't produce a memory leak or segfault in an intermediate step, so I added it as once. |
|
If you want to inspect the "new" flow how we export the buffers please start with the screencopy handlers here: |
|
Changing screenresolution can work, but it can fail, if pipewire let's us delete a buffer in the middle of processing (eg. after the copy request and before the buffer_ready, which segfaults in export_buffer). We probably need a check if the buffer we are exporting still exists. Further todo: investigate if we can just take any buffer as invalid and cleanedup after pipewire calls remove_buffer and we don't have to requeue them. |
|
We need to push a bugfix release before this lands (which may be sooner than I expected, thanks columbarius). People without configs who also don't have a default chooser installed are experiencing errors. |
|
This |
I second that part with the bugfix release. I don't see this MR merged soon, since the change in resolution/renegotiation part needs some work, where I'm not sure how to do it (also important to the point, that pipewire doesn't kills our buffers while we are using them) and I would like to make things right in a way, that it is nicely compatible with both shm and dmabuf depending on the clients preferences. I just had some unexpected time and made those changes to a point, where it works if you don't do anything exceptional. |
columbarius
force-pushed
the
screencast-pw-driven
branch
from
908d165
to
378cc32
Compare
|
Ok, i'm restarting the stream after a renegotiation by some kind of refcounting in {add,remove}_buffer. It's not elegant, but it works for now. Will have to modify obs again to test what's happening, if a client requests param changes. We might have to add a lock to the imported buffer and register the buffer_destroy call on the release of that lock, to prevent the buffer from being destroyed while wlroots is using it. We might be able to just drop that buffer without the requeueing the buffer to pipewire beforehand. Next step would be to rebase the/create a new dmabuf MR on top of the new flow. To test it I have a obs-studio branch ready (https://github.com/columbarius/obs-studio/tree/egl-modifiers). |
It should be fine to destroy the |
@emersion do you have an explanation how this error occures ( |
|
Can you grab a |
Now it's killed before by the address Sanitizer, but i noticed, that i'm not handling import_buffers well if the queue is empty. |
|
It seems like you're calling |
|
Maybe destroying the |
... i forgot to NULL pw_current_buffer and xdpw_simple_buffer.buffer if the import failed .... Nevermind still same error possible |
Still a use after free. |
|
Interesting, there may be a use-after-free in wlroots here. Can you run the compositor with ASan enabled? Maybe print what the stride values are inside wlroots? |
|
It has to be a use after free, because we destroy the buffer, while copy with damage is in progress. You are sure wlroots can/should handle that? (I'm asking, because that looks like sth. you should never do. Handing out a buffer to someone else and destroying while the other one is processing it.)
you mean compiling wlroots and sway with asan? |
Yeah, for instance it's valid for a client to
Yes. https://github.com/swaywm/sway/wiki/Development-Setup#compiling-as-a-subproject |
|
Ok, i printed the stride of the imported buffer in wlroots and they where different. I'm going forward with renaming the xdpw_frame to wlr_screencopy_frame (which will only hold the informations given by the wlr_screencopy protocol) and adding a "new" xdpw_frame, which holds the informations about the imported buffer from pipewire. This way we can check if the imported buffer is compatible with the bufferinformation advertised by wlroots. |
|
Thanks for cleaning up my poor naming conventions. |
Don't get your hopes too high up. I introduced a new xdpw_frame called simple_frame which is used in the interaction with pipewire, but also holds a wlr_buffer (that's why i didn't called it pipewire frame). |
|
Resolved those. I'm having problems with the fps limter while cleaning the stuff up. I will deactiveate before the flow change and activate it after wards. We might fix that later, or just ignore it in the intermediate commits. |
|
I don't wanna break master, but in this PR is fine. |
columbarius
force-pushed
the
screencast-pw-driven
branch
from
378cc32
to
cb890d1
Compare
|
I cleaned the patches up and restructured it a bit, so the steps are smaller and I think we are close to review. Screencast should work at every commit, renegotiation will be broken on some. |
columbarius
force-pushed
the
screencast-pw-driven
branch
from
cb890d1
to
a110a89
Compare
|
To be more explicit, I'm fine with broken commits on master for history preservation if they've never been at HEAD in a practical sense. Not sure if @emersion agrees, but if at rebase time, HEAD contains no regressions, I'm happy. |
… to interact with pipewire
Since we are driving the screencast there are no events on the pipewire loop calling the on_event callback. We want to import and export (if possible) on every frame of the wlroots loop, so this event is no longer needed.
This function lets us create a wl_buffer from any shared memory filedescriptor.
columbarius
force-pushed
the
screencast-pw-driven
branch
from
72f4bdc
to
da03200
Compare
|
rebased @emersion thanks for the partial review! |
This prevents the wlroots loop from running after the first roundtrip to query the buffer informations from the screencopy protocol.
…_screencopy_frame Since we gather the information of the currently used buffer on importing it, we can check if the imported and the announce buffer by wlroots are compatible. Also the comparison between the announced buffer properties and the format used by the pipewire stream was moved to wlr_frame_buffer_done. This lets us implement all checks in the same callback and makes it easier to extend those checks for future dmabuf sharing.
Instead of using one wl_buffer to export buffers from wlroots and then copy the content into the buffer dequed from pipewire, we can create a wl_buffer for each pipewire buffer directly at allocation time and attach it to the data attribute. Those wl_buffers can be directly handed over to the wlroots screencopy protocol and so removing one copy.
Move duplicated information to xdpw_frame and remove them from xdpw_screencopy_frame.
Sometimes it can happen that the first frame of the active stream triggers the renegotiation and destroy the frame without copy and with that starting the fps_limit counter. This triggers an assert in fps_limit_measure_end. To avoid it, we only engage the fps_limiter after a frame was copied successfully.
They callbacks are now in the order they are used by screencopy.
The enum xdpw_frame_state is used to track the state of the xdpw_frame through the screencopy callbacks. xdpw_wlr_stream_finish is used as the new endpoint of the screencopy callbacks. Here we clean up the screencopy_frame, enqueue the pipewire buffer and restart the screencopy loop if needed.
PipeWire can request to destroy the allocated buffers anytime. This isn't a problem for us, since the screencopy protocol can handle disappearing buffers. The only thing we have to do is not to use a destroyed buffer.
When a buffer is destroyed while used in the copy_buffer request the screencopy protocoll will answer with the failed event. This can happen anytime when PipeWire calls the remove_buffer callback, eg. on renegotiation. This is not fatal, so we don't need to close the screencast. cast->err should only be used for fatal errors.
We can trigger that with pw_stream_trigger_process when we are the driver of the stream. Additionally this let's us run passivly with the consumer driving the stream.
This tells pipewire, we prefere to use 4 pw_buffers in the pipe. This should remove most "out of buffer" occourences.
columbarius
force-pushed
the
screencast-pw-driven
branch
from
da03200
to
b224a75
Compare
| @@ -72,6 +72,9 @@ static void pwr_handle_stream_state_changed(void *data, | |||
| switch (state) { | |||
| case PW_STREAM_STATE_STREAMING: | |||
| cast->pwr_stream_state = true; | |||
| if (!cast->wlr_frame) { | |||
| xdpw_wlr_register_cb(cast); | |||
| } | |||
There was a problem hiding this comment.
Checked every commit again and found a race condition only in screencast: only restart wlroots loop if stream is active
when PipeWire sends a state_changed changing the state to PW_STATE_STREAMING in between the buffer_done and the ready/failed events of the screencopy loop. This did trigger a new screencopy loop entrance while the old wasn't finished and triggered an assertion in xdpw_pwr_dequeue_buffer.
Checking if NO screencopy loop is currently running via cast->wlr_frame == NULL solved this issue.
|
This doesn't affect anything else, so ready to merge. |
|
@columbarius Is this pull request related to your "cropping" branch? I've been running that branch for months ( |
|
@Hubro Not really. This MR is the preparation for hardware buffer support (DmaBuf). The cropping branch is blocked on adding cropping support on the chooser. I will write there what's open if anyone want's to pick it up. |
Is it an option to add it as xdg-desktop-portal-wlr specific configuration meanwhile? |
|
I'd rather not add temporary config options that we'll need to remove afterwards. |
I could add one in the patchset, but it won't be merged until all things are sorted out. |
This MR aims to implement screencast driven by pipewire as explained in #137.
Resolves: #137
Depends: #150Depends: #148Depends: #46Depends: #61