[RFC] DeepColor Visual Class Extension

[Adam Jackson]

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