[PATCH 1/2] hdr-metadata: Add protocol for static HDR metadata
Simon Ser
contact at emersion.fr
Mon Mar 11 20:15:59 UTC 2019
Hi,
Here are a few comments, from a protocol design perspective only.
On Monday, March 11, 2019 8:36 PM, Harish Krupo <harish.krupo.kps at intel.com> wrote:
> Signed-off-by: Harish Krupo <harish.krupo.kps at intel.com>
> ---
> Makefile.am | 1 +
> unstable/hdr-metadata/README | 4 +
> .../hdr-metadata/hdr-metadata-unstable-v1.xml | 104 ++++++++++++++++++
> 3 files changed, 109 insertions(+)
> create mode 100644 unstable/hdr-metadata/README
> create mode 100644 unstable/hdr-metadata/hdr-metadata-unstable-v1.xml
>
> diff --git a/Makefile.am b/Makefile.am
> index 345ae6a..8e70a9f 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -23,6 +23,7 @@ unstable_protocols = \
> unstable/xdg-decoration/xdg-decoration-unstable-v1.xml \
> unstable/linux-explicit-synchronization/linux-explicit-synchronization-unstable-v1.xml \
> unstable/primary-selection/primary-selection-unstable-v1.xml \
> + unstable/hdr-metadata/hdr-metadata-unstable-v1.xml \
> $(NULL)
>
> stable_protocols = \
> diff --git a/unstable/hdr-metadata/README b/unstable/hdr-metadata/README
> new file mode 100644
> index 0000000..5a53ac3
> --- /dev/null
> +++ b/unstable/hdr-metadata/README
> @@ -0,0 +1,4 @@
> +HDR metadata protocol
> +
> +Maintainers:
> +Harish Krupo <harish.krupo.kps at intel.com>
> diff --git a/unstable/hdr-metadata/hdr-metadata-unstable-v1.xml b/unstable/hdr-metadata/hdr-metadata-unstable-v1.xml
> new file mode 100644
> index 0000000..9c77fc9
> --- /dev/null
> +++ b/unstable/hdr-metadata/hdr-metadata-unstable-v1.xml
> @@ -0,0 +1,104 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="hdr_metadata_unstable_v1">
> +
> + <copyright>
> + Copyright © 2018 Intel
> +
> + 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="zwp_hdr_metadata_v1" version="1">
> + <description summary="HDR metadata">
> + The global interface used to instantiate a hdr surface from a wl_surface.
Style nit: "hdr" could be uppercase for consistency (it's an abbreviation).
> + The extended interface will allow setting the hdr metadata including
> + mastering display properties, content light level and the EOTF to the
> + surface.
> + </description>
> +
> + <request name="destroy" type="destructor">
> + <description summary="Destroy the object">
Style nit: "summary" properties are not capitalized.
> + Informs the server that the client will not be using this
> + protocol object anymore. This does not affect any other objects,
> + zwp_hdr_surface_v1 objects included.
> + </description>
> + </request>
> +
> + <enum name="error">
> + <entry name="hdr_surface_exists" value="0"
> + summary="the surface already has a hdr surface associated"/>
> + </enum>
> +
> + <request name="get_hdr_surface">
> + <description summary="Create an hdr surface">
> + Create an interface extension for the given wl_surface. This request
> + raises the HDR_SURFACE_EXISTS error if a zwp_hdr_surface_v1 is already
> + associated with the wl_surface.
> + </description>
> + <arg name="id" type="new_id" interface="zwp_hdr_surface_v1"
> + summary="the new hdr surface interface id"/>
> + <arg name="surface" type="object" interface="wl_surface"
> + summary="the surface"/>
> +
> + </request>
> + </interface>
> +
> + <interface name="zwp_hdr_surface_v1" version="1">
> +
> + <description summary="HDR SURFACE">
Style nit: this can be lowercase
> + An extension interface to the wl_surface object to set the HDR metadata
> + associated with the surface.
> + </description>
> +
> + <enum name="EOTF">
Style nit: this can be lowercase.
> + <entry name="ST_2084_PQ" value="0"
> + summary="SMPTE ST 2084:2014, HDR EOTF of Mastering Reference Displays"/>
> + <entry name="HLG" value="1"
> + summary="Hybrid Log-Gamma (HLG) based on ITU-R"/>
> + </enum>
> +
> + <request name="set">
> + <description summary="set the HDR metadata for a surface">
This is missing a description.
When is this new state applied? On next commit?
It would probably be better to have separate requests for each "group" of
metadata (display_primary, white_point, luminance and so on). This would prevent
having functions with 12 arguments and this would keep the protocol consistent
if it's extended.
> + </description>
> + <arg name="display_primary_r_x" type="fixed" summary="Red primary X"/>
> + <arg name="display_primary_r_y" type="fixed" summary="Red primary Y"/>
> + <arg name="display_primary_g_x" type="fixed" summary="Green primary X"/>
> + <arg name="display_primary_g_y" type="fixed" summary="Green primary Y"/>
> + <arg name="display_primary_b_x" type="fixed" summary="Blue primary X"/>
> + <arg name="display_primary_b_y" type="fixed" summary="Blue primary Y"/>
> + <arg name="white_point_x" type="fixed" summary="White point X"/>
> + <arg name="white_point_y" type="fixed" summary="White point Y"/>
> + <arg name="min_luminance" type="fixed" summary="Minimum luminance"/>
> + <arg name="max_luminance" type="fixed" summary="Maximum luminance"/>
> + <arg name="max_cll" type="uint" summary="Maximum content light level"/>
> + <arg name="max_fall" type="uint" summary="Maximum frame-average light level"/>
> + </request>
> +
> + <request name="set_eotf">
This is missing a description and a summary.
> + <arg name="eotf" type="uint"
> + summary="Electro optical transfer functions for the corresponding HDR transformation"/>
Does this take values from the "eotf" enum? If so, the <arg> should have a
enum="eotf" property.
> + </request>
> +
> + <request name="destroy" type="destructor">
> + <description summary="Destory the hdr metadata associated with the surface">
Typo: "destroy"
This is also missing a description. Is the HDR state removed on next commit?
> + </description>
> + </request>
> + </interface>
> +
> +</protocol>
> --
> 2.20.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list