Hi, Here are a few comments, from a protocol design perspective only.
On Monday, March 11, 2019 8:36 PM, Harish Krupo <harish.krupo....@intel.com> wrote: > Signed-off-by: Harish Krupo <harish.krupo....@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....@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
