[RFC] DeepColor Visual Class Extension

[Aaron Plattner]

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