On Sunday, April 14, 2019 1:57 PM, Erwin Burema <[email protected]> wrote:
Thanks for your patch! Here are a few comments, mostly from a protocol
perspective since I don't know about color management.

> ---
>  .../cm_wayland_calibration.xml                | 106 ++++++++++++++++++
>  1 file changed, 106 insertions(+)
>  create mode 100644 
> unstable/color-manager-calibration/cm_wayland_calibration.xml
>
> diff --git a/unstable/color-manager-calibration/cm_wayland_calibration.xml 
> b/unstable/color-manager-calibration/cm_wayland_calibration.xml
> new file mode 100644
> index 0000000..be9bb4b
> --- /dev/null
> +++ b/unstable/color-manager-calibration/cm_wayland_calibration.xml
> @@ -0,0 +1,106 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="cm_calibrate__profile_unstable_v1">

You can't use the "cm" prefix here. I assume this would have the "wp" prefix
instead.

> +  <copyright>
> +    Copyright © 2019 Erwin Burema
> +
> +    Permission is hereby granted, free of charge, to any person obtaining a
> +    copy of this software and associated documentation files (the 
> "Software"),
> +    to deal in the Software without restriction, including without limitation
> +    the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +    and/or sell copies of the Software, and to permit persons to whom the
> +    Software is furnished to do so, subject to the following conditions:
> +
> +    The above copyright notice and this permission notice (including the next
> +    paragraph) shall be included in all copies or substantial portions of the
> +    Software.
> +
> +    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> OR
> +    IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> +    THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> OTHER
> +    LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> +    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> +    DEALINGS IN THE SOFTWARE.
> +  </copyright>
> +
> +  <interface name="zcm_calibration_manager_v1" version="1">
> +      <description summary="Manager object for profiling/calibration of 
> displays">
> +          With this interface a compositor can announce 
> calibration/profiling support so a software can calibrate the display

Text should be wrapped at 80 chars.

> +          Currently this is just a sketch for a proposal
> +
> +          THIS INTERFACE IS EXPERIMENTAL!
> +      </description>
> +
> +      <request name="destroy" type="destructor">
> +        <description summary="destroy the calibration manager">
> +            Destroy the calibration manager
> +        </description>
> +      </request>
> +
> +      <request name="cm_output">
> +        <description summary="Request to calibrate/profile a given output">
> +          A program sends this request to a compositor to calibrate a given 
> output
> +
> +          A compositor can deny this request, in this case it should give an 
> error as defined in the error enum
> +        </description>
> +        <arg name="id" type="new_id" interface="zcm_output_cal_v1" />
> +        <arg name="output" type="object" interface="wl_output" />
> +      </request>
> +
> +      <enum name="error">
> +        <entry name="denied" value="0" summary="Given when the compositor 
> denies cal/profiling access" />

Errors have special semantics and are meant to be used only for protocol
violations, that is: a client sent an invalid request. They immediately
terminate the Wayland connection. Clients should always be able to know whether
a request will trigger a protocol error.

For this reason, it's probably better to remove this "denied" error and let
another mechanism/protocol deal with security.

> +        <entry name="not_saved" value="1" summary="Afer 
> calibration/profiling no profile or video card lut was saved" />

Would compositors always need to save the profile?

Since this is more of a runtime error, this probably shouldn't be a protocol
error.

> +      </enum>
> +
> +  </interface>
> +
> +  <interface name="zcm_output_cal_v1" version="1">
> +    <description summary="Main calibration profiling interface">
> +      With this interface an calibration/profiling application can send 
> needed colors to the display, the compositors is responsible for placing the 
> colors (with the help of the user since we can't know the physical location 
> of the measurement device)
> +    </description>
> +
> +    <enum name="bitdepth">
> +      <entry name="8bit" value="0" summary="8 Bit display" />
> +      <entry name="10bit" value="1" summary="10 Bit display" />
> +      <entry name="12bit" value="2" summary="12 Bit display" />
> +      <entry name="14bit" value="3" summary="14 Bit display" />
> +    </enum>
> +
> +    <event name="display_depth">
> +      <description summary="Event send to inform application of display 
> depth"/>
> +      <arg name="depth" type="uint" enum="bitdepth" />

Is this information enough to describe the display's capabilities? Is it really
required? Shouldn't this be in the regular color management protocol?

> +    </event>
> +
> +    <request name="set_color">
> +      <description summary="Request a color to be set to an area within 
> output">
> +        This request is semd to set a specific color to an output, this 
> color needs to be pushed to the screen as directly as possible (unless the 
> bool use_profile is true and with the exception of the HW LUT of the graphics 
> card or display itself) and preferably on top of everything else. The 
> compositor is responsible to place the output area inside the output.
> +      </description>
> +      <arg name="red" type="uint" summary="Red component of color, only 
> least significant n bit are used where n depends on bitdepth of display" />
> +      <arg name="green" type="uint" summary="Green component of color, only 
> least significant n bit are used where n depends on bitdepth of display" />
> +      <arg name="blue" type="uint" summary="Blue component of color, only 
> least significant n bit are used where n depends on bitdepth of display" />
> +      <arg name="use_profile" type="bool" allow-null="true" summary="If 
> profile should be applied, used to verify profile. If not set assume false"/>

We don't have bools. And allow-null can't be used with arguments that aren't
objects.

> +    </request>
> +
> +    <request name="load_profile">
> +      <description summary="Load a given profile including VCGT">
> +        USed to load a given ICC profile a compositor should also apply VCGT 
> to video card LUT. If this interface is destroyed without saving compositor 
> should restore previous settings
> +      </description>
> +      <arg name="profile" type="fd" summary="file descriptor to ICC 
> profile"/>
> +    </request>
> +
> +    <request name="save" type="destructor">
> +      <decsription summary="Last profile loaded should be saved">
> +        Send this to save the last profile loaded, compositor is allowed to 
> check this profile and deny the reqest (see error enum in manager interface). 
> irregardless of if profile gets saved or not this will destroy this 
> interface. If not saved compositor should restore previous settings.
> +      </description>
> +    </reqest>
> +
> +    <request name="desstroy" type="destructor">
> +      <description summary="Destroy object without saving" />
> +    </request>
> +
> +  </interface>
> +
> +
> +</protocol>
> --
> 2.21.0
>
> _______________________________________________
> wayland-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Reply via email to