Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

output: Add function to set preferred render bit depth #3162

Closed
wants to merge 1 commit into from
Closed

output: Add function to set preferred render bit depth #3162

wants to merge 1 commit into from

Conversation

mstoeckl
Copy link
Contributor

@mstoeckl mstoeckl commented Sep 3, 2021

This PR is an approach at #1378 . See for #2340 for earlier work in this direction. For more details, see the commit messages.

I'm fairly new to this area of wlroots; most of this PR was made by copying logic from other wlr_output properties.

The most important thing I am uncertain about is if the new framework to set the preferred bit depth is appropriate. One alternative to enum wlr_output_bit_depth might be an array of uint32_t fourcc buffer formats, in preference order. Alternatively, one could use WLR_OUTPUT_BIT_DEPTH_FAST or WLR_OUTPUT_BIT_DEPTH_MOST items to the enum to directly express users' likely preferences, but those might not be helpful if (future) Mesa offers the incomparable ABGR1616161616 and ABGR1616161616F , or if for some reason we want to render in YUV.

This is marked as WIP, since some details still need to be resolved:

  • To make backends request a new buffer if the render bit depth preference change would alter their buffer format.
  • The DRM backend needs to use the bit depth preference

Edit: the latest commit (output: recreate swapchain if format has changed) seems to fix the above two details.

(Misc: The "problem with the GL renderer" preventing 16-float bpc formats from being added is that to get screenshots working properly, one must use 'GL_HALF_FLOAT_NV' instead of 'GL_HALF_FLOAT_OES' for the output buffer.)


To test that this works, use the Sway PR swaywm/sway#6475 . The command output '*' render_bit_depth 10 should set 10-bit render depth., although if this is done at runtime, it will only take effect at the next buffer change.

I've checked this with the nested Wayland backend (which supports 10-bit output formats when run nested under Sway), and the X11 backend (which can only output 8-bit buffers). Setting WAYLAND_DEBUG=1 on the nested sway instance should reveal the buffer format being used. Alternatively, running grim will print unsupported format 808665665 or similar when the output uses 10 bpc. If you have Cairo built from git master, the grim PR emersion/grim#97 will produce 16-bit PNGs that can be used to confirm that 10-bpc buffers from clients work.

@emersion
Copy link
Member

emersion commented Sep 4, 2021

Thanks for working on this!

First off, I'm not yet sure what's the best way to rig up this functionality. In the future, I'd like to move all of the swapchain and rendering related functionality into a separate helper. Ref #3079.

I think there are two separate things that could be toggled when talking about 10bpc:

  1. The depth of the buffer we'll render to, as done in this PR. The backend doesn't need to know about it.
  2. The depth of the video stream on the wire, controlled by the max bpc DRM property. The backend needs to know about it.

Do we want these two to always be in sync?

I'd prefer to let compositors override the format, instead of adding a bit depth selection logic to wlr_output.

@mstoeckl
Copy link
Contributor Author

mstoeckl commented Sep 4, 2021

Do we want these two to always be in sync?

I sometimes use 16F or 10-bit depth, while sending 8-bit output on wire; this makes it possible for screenshots to capture the full detail of OpenGL windows producing 10-bit output. Color management, depending on how it is implemented and interacts with scanout, may also need to render to a 16F buffer. If hardware/software were to support temporal dithering, the render bit depth would best be slightly higher than the wire bit depth.

For high bit depth monitors, the best interface on the Sway side might be some extension of the mode string, since bandwidth limitations determine which bit-depth/size/fps/subsampling combinations are possible. For example, swaymsg output X mode 10240x4320@120Hz@12bpc@YCbCr420. In such a case, the proper thing to do would be to set the render format to the most efficient format which is at least 12 bit.

The DRM plane formats do not exactly overlap with e.g. the HDMI formats; there is no direct equivalent for 12-bpc RGB.

Right now, I'd prefer to have render and wire depth be decoupled. For compositors that wish to connect them, adding a helper function that provides a render depth preference based on the wire depth setting might suffice. (A possible problem here is that max_bpc is not active_bpc; the render depth should depend on the actual wire bpc, not the worst case.)

I'd prefer to let compositors override the format, instead of adding a bit depth selection logic to wlr_output.

So instead of

void wlr_output_set_preferred_bit_depth(struct wlr_output *output,  wlr_output_bit_depth depth);

something like

void wlr_output_set_format_preference_order(struct wlr_output *output, const uint32_t *format_list);

in which the compositor passes in the list of formats, and wlroots picks the first one compatible with the output and the renderer?

@mstoeckl
Copy link
Contributor Author

mstoeckl commented Sep 4, 2021

in which the compositor passes in the list of formats, and wlroots picks the first one compatible with the output and the renderer?

I've updated this branch to do this, as well as the Sway branch.

Edit: updated to rename WLR_OUTPUT_STATE_PREFERRED_DEPTH to WLR_OUTPUT_STATE_PREFERRED_FORMAT. Also, since everything seems to work when I test it, I've dropped the WIP/draft state.

@mstoeckl mstoeckl changed the title WIP: output: Add function to set preferred render bit depth output: Add function to set preferred render bit depth Sep 6, 2021
@mstoeckl mstoeckl marked this pull request as ready for review September 6, 2021 01:10
@emersion
Copy link
Member

emersion commented Sep 6, 2021

I'm wondering, should we just provide a function which takes a single format instead of a format list?

The DRM backend should already have the logic to demote ARGB to XRGB if the primary plane doesn't support it. Hm, maybe that would involve a bit more work, adding more opaque formats in the DRM backend to the get_primary_formats hook. At some point we should allow compositors to call get_primary_formats but this requires knowledge about the allocator.

@mstoeckl
Copy link
Contributor Author

mstoeckl commented Sep 7, 2021

I'm wondering, should we just provide a function which takes a single format instead of a format list?

This can be emulated with the format list, by providing an array with only one nonzero element, like {DRM_FORMAT_XRGB8888_A8, 0}. (Not having ARGB8888 somewhere in the preference list might break cursor plane format selection with the current PR, though.) On the other hand, the format list approach can probably be emulated with single formats and wlr_output_test, or via get_primary_formats and wlr_renderer_get_render_formats.

I'm weakly in favor of the format list approach right now, because it looks like the other options would increase the complexity of output handling in Sway even more.

The DRM backend should already have the logic to demote ARGB to XRGB if the primary plane doesn't support it.

Is there any reason to prefer ARGB over XRGB for the first primary plane, which should have nothing behind it? Running Weston, I find it uses XRGB2101010 for the primary, and ARGB8888 for cursor and overlay plane. Also, often only the opaque plane formats are supported; I have XRGB16161616F, but no ARGB16161616F.

@mstoeckl
Copy link
Contributor Author

I've updated this again, so that wlr_output_set_render_format_preference_order no longer affects the way the cursor format is chosen.

This change introduces new double buffered state to the wlr_output,
corresponding to a list which indicates which format of buffer to
render to. (If the backend and renderer do not support the first
format in the list, the second is tried, and so on.)

An output will by default pick ARGB8888 (or XRGB8888), as before.

The format being rendered to does not control the bit depth of
colors being sent to the display; it does generally determine
the format with which screenshot data is provided. The DRM backend
_may_ sent higher bits depths if the render format depth is
increased, but hardware and other limitations may apply.
@mstoeckl
Copy link
Contributor Author

Rebased, adjusting for fb393dd.

@emersion
Copy link
Member

emersion commented Nov 1, 2021

wlroots has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3162

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants