[RFC] DeepColor Visual Class Extension

[Adam Jackson]

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?

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.

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.

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

- ajax