-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
linux-pipewire: Reject invalid buffers #5614
linux-pipewire: Reject invalid buffers #5614
Conversation
Is this supposed to solve #5468 ? |
No |
b0d9355
to
8d39523
Compare
8d39523
to
6b37a2d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message typo: metadate
→ metadata
Could you expand a bit more the context around these CORRUPTED flags? The documentation I found is painfully small, and I don't think I understand when nor how this flag is used. Is it PipeWire itself that sets it? The compositor? Is skipping the frame the best strategy to handle it?
This flag is set by the producer, in our case the compositor. Afaik this flag can be used to mark "broken" buffers, without specifying what the issue is. xdpw will use that flag if a buffer copy by the compositor failed. Since we manage the buffer the memory won't be invalid but the content might be corrupt (filled with garbage). The v4l2 plugin [1] uses this flag for the same purpose. Since a buffer with this flag won't contain wanted contend I can't think of a better solution than skipping it. In my testing obs just reused the last imported texture and continued after a non corrupted frame was received. [1] https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/master/spa/plugins/v4l2/v4l2-utils.c#L1264 |
6b37a2d
to
9deed95
Compare
9deed95
to
b55a47d
Compare
Updated this to incorporate both meta and junk flags and added a revertible commit for current GNOME behaviour. The hinted documentation for ignoring the size in case of DMA-BUFs will be written by me probably this week, but got an ack from mesa devs. Will link the PipeWire MR here. |
Try to extend that at the same time. |
2967c4f
to
ef6f804
Compare
PipeWire documentation for ignoring the buffer size: https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1189 |
ab89cc4
to
088e4e6
Compare
088e4e6
to
9bc61fc
Compare
9bc61fc
to
0458799
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments, but this looks generally good to me.
I forgot to mention, but could you please rebase against the main branch, and remove the merge commits? |
66bc7d6
to
e141d82
Compare
It's gone. Probably didn't changed the strategy when using githubs update feature. |
e141d82
to
5758de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you split this in 2 separate commits - and it actually helped me review it - but I'd prefer if you could squash the second commit into the first one before merge. Please mention the same GNOME pre 43 behavior in the first commit's message. (The reason I prefer squashing is to not have any point in the commit history where the capture won't work with a subset of consumers.)
5758de0
to
711da88
Compare
Squashed the commit and added the note to the first commit message. Additionally I marked the workaround in code with a comment, such that it will be recognized as such and can be easily removed later. I'll continue to use multiple commits to ease development and review, but I'm totally fine to squash them just before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit: please use linux-pipewire:
as the commit prefix; and please start the tile in upper case. In other words, this: linux-pipewire: Reject invalid buffers
PipeWire supports two flags to signal an invalid buffer: SPA_META_HEADER_FLAG_CORRUPTED signals that the whole buffer is invalid and should not be used SPA_CHUNK_FLAG_CORRUPTED signals that one single buffer plane is invalid Skipping a buffer because of size 0 was moved to only the SHM case. For DMA-BUFs the size of a single plane is not relevant and should be ignored [1]. Compatibility note: GNOME pre 43 sets the chunk size to 0 when a buffer copy failed. Luckily GNOME doesn't use the META_Header and thus we can detect if we should use the new or old style of invalid buffer detection. This workaround should be dropped when reasonable. [1] https://docs.pipewire.org/page_dma_buf.html
711da88
to
7e529e2
Compare
Done ... |
Heh the good, old, simpler times 🙂 |
This change spams my log with corruption messages even though it seems to be capturing fine. Fedora 37 |
@jp9000 is it |
@GeorgesStavracas thanks for merging! |
@columbarius It's |
Description
This patch checks the buffer received from PipeWire for flags indicating that the buffer is corrupt.
Motivation and Context
Portals can set a buffer as corrupted via flags. This allows obs to skip those frames and avoid trying to import them.
How Has This Been Tested?
Modified version of xdg-desktop-portal-wlr to alternate between those flags for a given time duration. With this patch your screencast will freeze until a new frame without a flag arrives. Mouse pointer should still be updated when it is not embedded.
Types of changes
Checklist: