On Mon, 2017-08-14 at 19:17 -0700, Alex Goins wrote: > 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.
Setting things to 0 that used to be reliably not 0 still makes me a little nervous, it tends to be a good way to provoke the client into either dividing by zero or transforming malloc(0) into a failure. Or, just violating other invariants. tk has this little gem when mapping from color name to pixel value: if (stressPtr->numColors == 0) { Tcl_Panic("FindClosestColor ran out of colors"); } ... where numColors is filled in from ->colormap_size in the XVisualInfo. Granted there are probably few tk apps in the world that would like to take advantage of HDR, but any amount of mandatory toolkit change is going to make adoption harder. Some of this can probably be avoided by ensuring that the enum for DeepColor is an even number (see comment in <X11/X.h>), but I might still be cautious and set this to 1 in practice and assert that it's undefined in the spec. > 3. Colorspace/Encoding Window Properties > > Windows with DeepColor visuals must rely on window properties, as opposed to > colormaps, to determine the relationship between pixel values and colors. > These > properties must specify constants that correspond to colorspace/encoding > pairs. > > Possible colorspace/encoding constants are defined to be: > > // Undefined: > // Undefined colorspace and encoding > #define _DEEPCOLOR_UNDEFINED 0 This may want a note saying this is just a sentinel value and not something you actually want to set on your window. Certainly the compositor is not going to know how to properly present "no colorspace". > 3.1.1. Root Window Properties > > Servers that support the DeepColor extension MUST, when initialized, create a > child of the root window named "DEEPCOLOR_PROPERTIES". Name as in the WM_NAME property I assume. > 3.1.2. "DEEPCOLOR_PROPERTIES" Window Properties > > The DeepColor extension defines a window property associated with > "DEEPCOLOR_PROPERTIES" for determining the set of colorspaces/encodings > supported by the compositor: > > _DEEPCOLOR_COMPOSITOR_COLORSPACES, colorspace, score, CARDINAL[][2]/32 > > This is a list of 2-tuples indicating the colorspaces/encodings supported by > the > compositor (whether internal or external to the server), and their level of > preference as indicated by their score, where higher is better. The server > MUST > initialize the property with a list of colorspaces/encodings supported by its > internal compositor, which may be an empty list if no DeepColor capabilities > are > supported. As discussed in section 3.2, applications MUST choose a > colorspace/encoding from this set, if non-empty. Would there be any benefit to making these a 3-tuple of { colorspace, pixel format, score } ? Thinking there might be hardware where FP16 is enough more painful than 10bpc that performance would be unacceptable. Though, if that were true, that's almost certainly true regardless of the associated colorspace, so maybe pixel formats just want a separate priority list, or maybe there are so few pixel formats that it's obvious what to do. > If an external application becomes responsible for compositing and supports > the > DeepColor extension, it MUST override this property with its own supported > colorspaces/encodings prior to calling XCompositeRedirectSubwindows on the > root > window. The server MUST delay sending the PropertyNotify event for the change > in > _DEEPCOLOR_COMPOSITOR_COLORSPACES until after the root window hierarchy has > been > redirected. The only way to implement this at present is for the driver to override ProcChangeProperty in the dispatch vector, which is... distasteful. I really think this should be a new request that the compositor must make before redirecting subwindows: DPCCOLORSPACE: [ colorspace: CARD32 score: CARD32 ] DPCSetCompositeColorspace: window: WINDOW colorspace_list: LISTofDPCCOLORSPACE If we remember that redirection can happen anywhere in the hierarchy, we could imagine a DEEPCOLOR_PROPERTIES window at any level. With a new request, the new deepcolor properties begin life as client state, are applied to the CompSubwindows state when CompositeRedirectSubwindows actually happens, and would naturally apply to any level of nested composition. This is probably overkill to actually implement, but it seems instructive to consider how it "would" work if nested HDR was supported. > If XCompositeRedirectSubwindows is requested for the root window without being > preceded by a corresponding change to _DEEPCOLOR_COMPOSITOR_COLORSPACES, the > server MUST assume that the external compositor making the request is unaware > of > the DeepColor extension, clearing the property to indicate the loss of > compositor DeepColor support. With a new request this is natural, either the compositor called it or not and we saved that in client state. If we just hack event delivery to know about the new property, Composite has to be taught to go grub around in a delayed event queue we don't already have. > The colorspace/encoding of the compositor's target surface MUST be a member of > the set supported by the server for display. In order for an external > compositor > to be aware of the set of colorspaces/encodings supported by the server for > display, the DeepColor extension defines another window property associated > with > "DEEPCOLOR_PROPERTIES": > > _DEEPCOLOR_DISPLAY_COLORSPACES, colorspace, score, CARDINAL[][2]/32 > > This is a list of 2-tuples indicating the set of colorspaces/encodings > supported > by the server for display, and their level of preference as indicated by their > score, where higher is better. The server MUST initialize the property with a > list of colorspaces/encodings supported for display, which may be an empty > list > if no DeepColor capabilities are supported. > > The set of supported colorspaces MUST NOT change after being first > initialized, > and MUST be used by an external compositor to determine limits on the > colorspace/encoding of its target surface. We don't currently have an 'immutable' bit for properties, so there's nothing at the moment preventing a client from smashing this property itself. Fixable, just, a thing. The question about adding pixel format to the tuple applies here too. > DPCGetVisualInfo > > visual_list: LISTofVISUALID > => > per_visual_info: LISTofVISUALINFO > > where: > > 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, Something seems funny to me about including depth-30 visuals here, when they'd be perfectly representable as TrueColor visuals. I suppose the intent is to hide them from non-HDR-aware applications since otherwise a naïve search for the visual with the most RGB bits could land on a visual with worse performance. Probably not actually a problem, just seemed curious. This spec makes no provision for creating >32bpp pixmaps. We could infer that they're possible from this pixel format list, but then doing anything with them will be impossible until a core/Render interaction is defined. > 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. > Furthermore, clients that are not aware of the new visual class may trip over > it > and crash. > * The current standing suggestion for addressing this problem involves > having > DeepColor visuals exposed in the core protocol as TrueColor visuals, with > anything pertaining to DeepColor queryable only through DPCGetVisualInfo, > including the "true" visual class. Normal TrueColor visuals would reflect > TrueColor in the extended visual info, but DeepColor visuals would reflect > DeepColor, along with pixel format. Core protocol requests using drawables > with this visual would treat them as TrueColor, with the server using its > knowledge of the true format to convert where necessary, with operations > restricted to GXCopy, planemask ~0. > > One standing issue with this idea is that from the core protocol's > perspective, these masquerading DeepColor/TrueColor visuals would appear > to > be identical to the "true" TrueColor visuals, as well as other > masquerading > DeepColor/TrueColor visuals that have different pixel formats. While many > clients may traverse the list of visuals linearly, picking the first match > from the set of visuals obtained through the connection block, this is > not a > given. If a naive client wrongly picks a masquerading visual, we incur > undue > overhead doing unnecessary conversions, and maybe even corruption if the > conversion if imperfect or lossy. I think I'm convinced enough that a new visual class can be safe, and can at any rate be hidden by libX11 if necessary. We need to make at least _some_ strong recommendation about the interaction with the rest of rendering. At a minimum, the window background pixel/pixmap is going to get painted by the server before the application has the opportunity to define real window content, so that wants to look reasonably close to what you'd get on a TrueColor window. (And Render wants at least a new internal PictFormat, or else to be nerfed entirely for DeepColor visuals.) > Due to a suggestion to move to constants instead of atoms for defining > supported > colorspaces, and the resulting requirement that additional colorspaces are > added > through revisions to the DeepColor spec, the set of colorspaces defined in the > first version of the spec has been expanded to cover every colorspace that is > suspected to be in demand in the immediate future. The BT.2020 HLG and ACES > colorspaces are currently not supported by the requisite EGL/Vulkan > extensions; > that support will have to be added before drivers/the server can support them. One possibility would be to pass back the highest-numbered colorspace the implementation knows about in DPCQueryVersion. Or just go ahead and make that number _be_ the minor version. - ajax _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel