[PATCH 1/2] hdr-metadata: Add protocol for static HDR metadata
Harish Krupo
harish.krupo.kps at intel.com
Mon Mar 11 20:53:40 UTC 2019
Hi Simon,
Thanks for the comments. Please find my comments inline.
Simon Ser <contact at emersion.fr> writes:
> 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).
Yes, sure.
>
>> + 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.
>
Okay.
>> + 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
>
Yup.
>> + 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.
>
Enum names 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?
>
Yes, the state setting is double buffered. It is applied on
wl_surface::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.
>
Yes, I agree. There is a protocol for color management which is
currenlty under discussion in the mailing list. The protocol would share
these primaries, white point details as icc profiles using file fds. We
intend to incorporate the luminance information and mastering chromacity
information into the same protocol.
>> + </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.
>
Agreed.
>> + </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?
>
Yes, the HDR state is reset on destroying the object.
Thanks again for reviewing. As this is just a provisional protocol, we
didn't give much heed to the exact details. I agree that it shouldn't be
the case. I will update this protocol with more details and send in v2.
Regards
Harish Krupo
More information about the wayland-devel
mailing list