On 07/18/2017 07:18 AM, Adam Jackson wrote: > On Mon, 2017-07-17 at 18:04 -0700, Alex Goins wrote: >> This is our latest iteration on the design of the visual class for use with >> HDR >> drawables, handling how colorspace/encoding and pixel format are specified >> and >> interpreted. We've been brainstorming this internally for a while, taking >> into >> consideration comments that came up in the xorg-devel thread several months >> back. We've now formalized it as an X extension. >> >> This extension is designed to be flexible as to how HDR drawables are >> actually >> implemented, leaving it up to the server / DDX drivers to decide. It should >> be >> capable of working interchangeably when an external compositor is being used >> and >> when one is not, with the server/driver being responsible for internal >> composition in the latter case. >> >> It should be capable of working with GLX, EGL, and Vulkan, keeping track of >> colorspace/encoding and pixel format in API-agnostic locations so that >> compositors need not rely on API-specific methods for querying these >> properties. >> Supported colorspaces/encodings were chosen as a superset of those supported >> by >> the requisite EGL/Vulkan extensions, with GLX relying entirely on the X >> properties for the purpose of determining colorspace/encoding. >> >> Let us know what you think! > > Apologies for the brief reply, I'm about to be on PTO for a bit so this > is a little rushed. In general this looks quite good on a first read- > through, I'll let it sink in a bit while I'm away. > >> 2. DeepColor Visual Class >> >> The DeepColor extension defines a new visual class, DeepColor. >> >> DeepColor visuals do not make use of the red_mask, green_mask, blue_mask, or >> colormap_size fields of the XVisualInfo structure. Colormap size is defined >> to >> be 0. > > This logically makes sense, but I'm slightly nervous about exposing > clients to an xVisualType they've never seen before. Would it make > sense to hide this list from the connection setup block and only return > it in DPCGetVisualInfo?
I don't think we can hide it completely, since XGetWindowAttributes will return what looks like a bogus visual then. We went through something similar with XLIB_SKIP_ARGB_VISUALS -- maybe something like that would work here? > Alternatively, would it make sense to fill in those masks with dummy > (non-zero) values that look like xrgb32 so we don't trigger a divide- > by-zero in a weird place? If we're going to ignore the values anyway... > >> 3.1. Root Window Properties >> >> The DeepColor extension defines a root window property for determining the >> set >> of colorspaces/encodings supported by the X server or external compositor: >> >> _DEEPCOLOR_COMPOSITOR_COLORSPACES, ATOM[] > > I didn't like this for style reasons when I first read through. The > problem with root window properties is that every client ends up > listening for PropertyNotify on the root window, so every change to any > property wakes up every client. Which... > >> 7. Issues >> >> This spec does not address how HDR content should be downsampled to SDR >> content >> for e.g. automatic redirection, XGetImage(), or core X11 / RENDER rendering. >> Perhaps the server could handle this transparently without the need for >> explicit functionality outlined in the spec. >> >> This spec does not address what should happen to >> __DEEPCOLOR_COMPOSITOR_COLORSPACES when an HDR-unaware compositor is >> started, as >> it probably should. > > ... kind of matters here. I think what should happen is, if the > compositor thinks it's going to be HDR-aware, it sets that property > before CompositeRedirectSubwindows for the screen, but the server > delays sending that property change event until afterwards. That way if > you (the server) see RedirectSubwindows without a property change > pending, you know to clear the colorspace list. And clients never see > an inconsistent state about which colorspaces will work. Clever. > To avoid doing too much surgery to how core property changes work we > could add this as a declaration-of-intent request to DEEP-COLOR: > > DPCSendPendingColorspaces > 1 CARD8 opcode > 1 X DPC opcode > 2 unused > 4 n number of ATOMs in list > 4n ATOM colorspaces > > But: whether that colorspace list is cleared or changed, every client > wakes up to notice. One way around this is to create a child window of > the root that lives only to hold the deep color properties, and point > to it with a root window property (that then never changes). > > This isn't exactly the first instance of this problem. I proposed > adding property notify filters to fixes a while ago, which would > address this whole category of problem. This might be a nice excuse to > get that landed so we can stop making the problem worse. Do you have any measurements of how much real-world pain this causes? >> VISUALINFO: [ core_visual_id: VISUALID >> pixel_format: CARD32 ] >> >> - pixel_format is an enumeration of pixel formats: >> FP_R16G16B16A16 = 0, >> UINT_R16G16B16A16, >> UINT_A2R10B10G10, >> UINT_A2B10G10R10, > > Is it worth doing the Atom trick for naming these? Maybe? I think for both this and the colorspaces property, it's important to allow future revisions of the spec to add new ones without existing clients breaking. But unlike _DEEPCOLOR_COMPOSITOR_COLORSPACES, if there are visuals that the compositor can't understand because they were added in a later spec, I'm not sure how to deal with that. > - ajax _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
