linux-pipewire: Reject invalid buffers

[columbarius]

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

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code has been run through clang-format.
• I have read the contributing document.
• My code is not on the master branch.
• The code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[kkartaltepe]

Is this supposed to solve #5468 ?

[columbarius]

Is this supposed to solve #5468 ?

No

[GeorgesStavracas]

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?

[columbarius]

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

[columbarius]

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.

[columbarius]

The documentation I found is painfully small

Try to extend that at the same time.

[columbarius]

PipeWire documentation for ignoring the buffer size: https://gitlab.freedesktop.org/pipewire/pipewire/-/merge_requests/1189

[GeorgesStavracas]

Two comments, but this looks generally good to me.

[GeorgesStavracas]

I forgot to mention, but could you please rebase against the main branch, and remove the merge commits?

[columbarius]

I forgot to mention, but could you please rebase against the main branch, and remove the merge commits?

It's gone. Probably didn't changed the strategy when using githubs update feature.

[GeorgesStavracas]

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.)

[columbarius]

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.)

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.

[GeorgesStavracas]

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

[columbarius]

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

Done ... linux-capture... what a nod from the past ^^`

[GeorgesStavracas]

Done ... linux-capture... what a nod from the past ^^`

Heh the good, old, simpler times 🙂

[jp9000]

This change spams my log with corruption messages even though it seems to be capturing fine. Fedora 37

[columbarius]

@jp9000 is it [pipewire] buffer is corrupt or [pipewire] buffer contains corrupted data ?
The second one should also appear when the buffer only contains an cursor update without new screen data. Maybe we should change the loglevel or message.

[columbarius]

@GeorgesStavracas thanks for merging!

[jp9000]

@columbarius It's [pipewire] buffer contains corrupted data, the one on line 533. I just noticed it happens if I have OBS on a monitor different from the one being captured. If OBS is capturing the same monitor it's fine, but when I move it onto my other monitor, it starts spamming that in the logs in spite of seemingly capturing fine.