Skip to content

Revert "linux-pipewire: Fix 10- and 16-bit captures"#12422

Merged
RytoEX merged 1 commit into
obsproject:masterfrom
tytan652:revert-pipewire10b
Jul 25, 2025
Merged

Revert "linux-pipewire: Fix 10- and 16-bit captures"#12422
RytoEX merged 1 commit into
obsproject:masterfrom
tytan652:revert-pipewire10b

Conversation

@tytan652
Copy link
Copy Markdown
Collaborator

Description

Fixes #12412

This reverts commit 4dfd0b3.

Also adds a FIXME comment.

Motivation and Context

#11207 generated issues on SDR users.
And the PR itself tries to handle color space the wrong way, the portal implementation should provide a node with PipeWire colorimetry info in its formats for OBS Studio to handle textures correctly.

How Has This Been Tested?

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.

@tytan652 tytan652 added this to the OBS Studio 31.1 milestone Jul 22, 2025
@tytan652 tytan652 added kind/bug Categorizes issue or PR as related to a bug. platform/linux Categorizes issue or PR as affecting Linux specifically labels Jul 22, 2025
@PancakeTAS
Copy link
Copy Markdown
Contributor

Merging this would break capturing normal SDR 10-bit and 16-bit with pipewire. Why should this be merged?

@tytan652
Copy link
Copy Markdown
Collaborator Author

tytan652 commented Jul 22, 2025

Merging this would break capturing normal SDR 10-bit and 16-bit with pipewire. Why should this be merged?

#11207 has secondary effect that are considered regressions in 31.1. Unfortunately, I don't think easily fixing with a few lines of code is possible. So revert has to be considered.

@PancakeTAS
Copy link
Copy Markdown
Contributor

Unfortunately, I don't think easily fixing with a few lines of code is possible.

I disagree. Take a look at video_colorspace_from_spa_color_matrix in line 602 of pipewire.c. This hardcodes all supported color spaces to these few, likely because the original pipewire code does not support any other to begin with (though I couldn't find the source code responsible for this).

Next, take a look at formats.c, the method obs_pw_video_format_from_spa_format here converts spa to obs again, this time it's the format rather than the color space.
As you can see from the array above, it too only supports a single 10-bit color format.

From my understanding, an image is non-linear only if it's either using a 10-bit format, or HDR PQ/HLG color space.

HDR isn't supported, so the only check that needs to be done is checking for a 10-bit color format.

So revert has to be considered.

Have you considered my branch posted in the original issue? On my machine, this fixes all the issues reported and I would like testing on compositors with hardware cursor support (as NVIDIA and hardware cursors don't really do well together). I can open a pull request right away, or wait for you or other people to test this. Just let me know!

Of course I totally understand why a revert is appropriate as well.

@tytan652
Copy link
Copy Markdown
Collaborator Author

I disagree. Take a look at video_colorspace_from_spa_color_matrix in line 602 of pipewire.c. This hardcodes all supported color spaces to these few, likely because the original pipewire code does not support any other to begin with (though I couldn't find the source code responsible for this).

This is not used by the ScreenCast portal code, this is for the Camera one.

@tytan652
Copy link
Copy Markdown
Collaborator Author

tytan652 commented Jul 22, 2025

Have you considered my branch posted in the original issue? On my machine, this fixes all the issues reported and I would like testing on compositors with hardware cursor support (as NVIDIA and hardware cursors don't really do well together). I can open a pull request right away, or wait for you or other people to test this. Just let me know!

I missed it. You should open a PR that as an alternative fix for the mentioned issue to attract more eyes, if you wish to go on that journey.

Copy link
Copy Markdown
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this chunk not need to be reverted?
4dfd0b3#diff-17471d738678ee9da05aa5f7e85201218d673609366b23cf07ddca571fd28ba7L794-R797

Contemplating if this should just be a clean revert.

@tytan652 tytan652 force-pushed the revert-pipewire10b branch from bc4c501 to f4b8b29 Compare July 25, 2025 18:03
This reverts commit 4dfd0b3.

Also adds a FIXME comment.
@tytan652 tytan652 force-pushed the revert-pipewire10b branch from f4b8b29 to 224ca18 Compare July 25, 2025 18:04
@tytan652
Copy link
Copy Markdown
Collaborator Author

I somehow forgot to keep that part in the revert.

Copy link
Copy Markdown
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I think this would be fine as a clean revert without the FIXME (to ostensibly make it easier to re-apply the original commit and then fixes for it), this is probably fine until #12424 is finalized.

@RytoEX RytoEX merged commit 24597a7 into obsproject:master Jul 25, 2025
15 checks passed
@tytan652 tytan652 deleted the revert-pipewire10b branch July 26, 2025 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. platform/linux Categorizes issue or PR as affecting Linux specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Render Delay filter on Pipewire Screen Capture causes gamma shift

3 participants