[RFC] DeepColor Visual Class Extension

[Aaron Plattner]

On 09/11/2017 09:10 AM, Adam Jackson wrote:
> 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.

If the window is identified by an XID property on the root window, it
doesn't seem like it necessarily needs a WM_NAME. Alex would it make
sense to just drop this from the spec, and then we can still add a
WM_NAME in the implementation for convenience? I would hate for
applications to get the wrong idea and try to find this window by
walking the window tree instead of just looking up the root window property.

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

It's not clear to me exactly how nested HDR should work. It seems to me
like you'd want to be able to add HDR support to some window (e.g. the
Blender 3D modeling windows or GIMP's image window) and have its HDRness
punch through the window tree without having to change the toolkit to
draw all the menus and buttons and whatnot in HDR too. But maybe that's
not a big deal?

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

How this is handled seems like a server implementation detail, but I
guess adding an extension request doesn't cost much given that we're
already adding a new extension anyway.

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

You could still use GLX with >32bpp pixmaps.

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

I'm worried that having to deal with the details of getting this
illusion completely right is going to be a distraction toward the goal
of getting full-screen HDR games or video to work.

I guess if push comes to shove, we could ship something that implements
the rest of the spec with broken or missing SDR conversion support,
behind an option you have to explicitly enable.

-- Aaron

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