unstable: add HDR Mastering Meta-data Protocol

Submitted by Nautiyal, Ankit K on Feb. 27, 2019, 4:57 a.m.

Details

Message ID 1551243436-15011-1-git-send-email-ankit.k.nautiyal@intel.com
State Superseded
Headers show
Series "unstable: add HDR Mastering Meta-data Protocol" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Nautiyal, Ankit K Feb. 27, 2019, 4:57 a.m.
From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

This protcol enables a client to send the hdr meta-data:
MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
SMPTE ST.2086.
The clients get these values for an HDR video, encoded for a video
stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
pixel in the entire stream/file in nits.
MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
average brightness in nits for a single frame. Max and Min Luminance
tells the max/min Luminance for the mastering display.
These values give an idea of the brightness of the video which can be
used by displays, so that they can adjust themselves for a better
viewing experience.

The protocol depends on the Color Management Protocol which is still
under review. There are couple of propsed protocols by Neils Ole [1],
and Sebastian Wick [2], which allow a client to select a color-space
for a surface, via ICC color profile files.
The client is expected to set the color space using the icc files and
the color-management protocol.

[1] https://patchwork.freedesktop.org/patch/134570/
[2] https://patchwork.freedesktop.org/patch/286062/

Co-authored-by: Harish Krupo <harish.krupo.kps@intel.com>
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
---
 Makefile.am                                        |  1 +
 unstable/hdr-mastering-metadata/README             |  5 ++
 .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 unstable/hdr-mastering-metadata/README
 create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml

Patch hide | download patch | download mbox

diff --git a/Makefile.am b/Makefile.am
index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml	\
 	$(NULL)
 
 stable_protocols =								\
diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
new file mode 100644
index 0000000..b567860
--- /dev/null
+++ b/unstable/hdr-mastering-metadata/README
@@ -0,0 +1,5 @@ 
+HDR-MASTERING-META-DATA-PROTOCOL
+
+Maintainers:
+Ankit Nautiyal <ankit.k.nautiyal@intel.com>
+Harish Krupo <harish.krupo.kps@intel.com>
diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
new file mode 100644
index 0000000..aeddf39
--- /dev/null
+++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
@@ -0,0 +1,95 @@ 
+<?xml version="1.0" encoding="UTF-8"?>
+<protocol name="hdr_mastering_metadata_unstable_v1">
+
+  <copyright>
+    Copyright © 2019 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>
+
+  <description summary="hdr mastering meta data protocol">
+    This protocol provides the ability to specify the mastering color volume
+    metadata for an HDR video, for a given surface.
+    These values give an idea of the brightness of the video, which can be
+    used by the display so that it can adjust itself for a better viewing
+    experience.
+
+    The hdr-metadata values are enocoded in the video and the client can
+    retreive these values and provide them to the compositor.
+
+    A client need to first get the color-space interface for the HDR
+    color-space using the color-manager protocol, via ICC profile.
+    Then it needs to get the hdr_mastering_surface using
+    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
+    hdr_surface interface which can be used to set the hdr mastering meta-data.
+  </description>
+
+  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
+    <description summary="hdr mastering metadata">
+      The hdr matering metadata is a singleton global object that provides
+      the extenstion hdr_surface for a given surface.
+    </description>
+
+    <enum summary="error">
+      <entry name="hdr_surface_exists" value="0"
+	      summary="The hdr surface exists for given surface."/>
+    </enum>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the hdr_mastering_metadata object">
+        Destroy the HDR mastering metadata object.
+      </description>
+    </request>
+
+    <request name="get_hdr_surface">
+      <description summary="get the interface for hdr_surface">
+        This interface is created for a surface and the hdr mastering metadata
+	should be attached to this surface.
+      </description>
+      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
+      <arg name="surface" type="object" interface="wl_surface"/>
+    </request>
+
+  </interface>
+
+  <interface name="zwp_hdr_surface_v1" version="1">
+    <description summary="an interface to add hdr mastering metadata">
+      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
+      for a given surface.
+    </description>
+
+    <request name="set_hdr_mastering_metadata">
+      <description summary="set the hdr mastering metadata for the surface.">
+        This request is double buffered and it will be applied to the surface
+	on wl_surface::commit.
+      </description>
+      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
+      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
+      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
+      <arg name="min_lum" type="uint" summary="MIN Luminance"/>
+    </request>
+
+    <request name="destroy" type="destructor">
+      <description summary="destroy the hdr mastering surface object">
+        Destroy the hdr_surface.
+      </description>
+    </request>
+  </interface>
+</protocol>

Comments

Hi,

On Wed, 27 Feb 2019 at 05:47, Nautiyal, Ankit K
<ankit.k.nautiyal@intel.com> wrote:
>
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>
> This protcol enables a client to send the hdr meta-data:
> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> SMPTE ST.2086.
> The clients get these values for an HDR video, encoded for a video
> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
> pixel in the entire stream/file in nits.
> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
> average brightness in nits for a single frame. Max and Min Luminance
> tells the max/min Luminance for the mastering display.
> These values give an idea of the brightness of the video which can be
> used by displays, so that they can adjust themselves for a better
> viewing experience.
>

This does sound quite good for video players (and in the future image
viewers), but might have something missing for HDR content generation
(Blender, Krita, Natron, etc) since you do not always know these
values in advance (it is effectively before mastering), most of these
work in scene linear with the convention that 0.18 is middle grey
(although this is just a convention). So I think that in these cases
we might need to get info from the display system on what luminance
levels its supports.

Hope this makes sense since at the way to busy with other obligations
so not much time to look into this.

> The protocol depends on the Color Management Protocol which is still
> under review. There are couple of propsed protocols by Neils Ole [1],
> and Sebastian Wick [2], which allow a client to select a color-space
> for a surface, via ICC color profile files.
> The client is expected to set the color space using the icc files and
> the color-management protocol.
>
> [1] https://patchwork.freedesktop.org/patch/134570/
> [2] https://patchwork.freedesktop.org/patch/286062/
>
> Co-authored-by: Harish Krupo <harish.krupo.kps@intel.com>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  Makefile.am                                        |  1 +
>  unstable/hdr-mastering-metadata/README             |  5 ++
>  .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 unstable/hdr-mastering-metadata/README
>  create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>
> diff --git a/Makefile.am b/Makefile.am
> index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml  \
>         $(NULL)
>
>  stable_protocols =                                                             \
> diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
> new file mode 100644
> index 0000000..b567860
> --- /dev/null
> +++ b/unstable/hdr-mastering-metadata/README
> @@ -0,0 +1,5 @@
> +HDR-MASTERING-META-DATA-PROTOCOL
> +
> +Maintainers:
> +Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> +Harish Krupo <harish.krupo.kps@intel.com>
> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> new file mode 100644
> index 0000000..aeddf39
> --- /dev/null
> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> @@ -0,0 +1,95 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="hdr_mastering_metadata_unstable_v1">
> +
> +  <copyright>
> +    Copyright © 2019 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>
> +
> +  <description summary="hdr mastering meta data protocol">
> +    This protocol provides the ability to specify the mastering color volume
> +    metadata for an HDR video, for a given surface.
> +    These values give an idea of the brightness of the video, which can be
> +    used by the display so that it can adjust itself for a better viewing
> +    experience.
> +
> +    The hdr-metadata values are enocoded in the video and the client can
> +    retreive these values and provide them to the compositor.
> +
> +    A client need to first get the color-space interface for the HDR
> +    color-space using the color-manager protocol, via ICC profile.
> +    Then it needs to get the hdr_mastering_surface using
> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
> +    hdr_surface interface which can be used to set the hdr mastering meta-data.
> +  </description>
> +
> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
> +    <description summary="hdr mastering metadata">
> +      The hdr matering metadata is a singleton global object that provides
> +      the extenstion hdr_surface for a given surface.
> +    </description>
> +
> +    <enum summary="error">
> +      <entry name="hdr_surface_exists" value="0"
> +             summary="The hdr surface exists for given surface."/>
> +    </enum>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the hdr_mastering_metadata object">
> +        Destroy the HDR mastering metadata object.
> +      </description>
> +    </request>
> +
> +    <request name="get_hdr_surface">
> +      <description summary="get the interface for hdr_surface">
> +        This interface is created for a surface and the hdr mastering metadata
> +       should be attached to this surface.
> +      </description>
> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +    </request>
> +
> +  </interface>
> +
> +  <interface name="zwp_hdr_surface_v1" version="1">
> +    <description summary="an interface to add hdr mastering metadata">
> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
> +      for a given surface.
> +    </description>
> +
> +    <request name="set_hdr_mastering_metadata">
> +      <description summary="set the hdr mastering metadata for the surface.">
> +        This request is double buffered and it will be applied to the surface
> +       on wl_surface::commit.
> +      </description>
> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>
> +    </request>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the hdr mastering surface object">
> +        Destroy the hdr_surface.
> +      </description>
> +    </request>
> +  </interface>
> +</protocol>
> --
> 2.7.4
>
Hi,

On 2/27/2019 2:28 PM, Erwin Burema wrote:
> Hi,
>
> On Wed, 27 Feb 2019 at 05:47, Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com> wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> This protcol enables a client to send the hdr meta-data:
>> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
>> SMPTE ST.2086.
>> The clients get these values for an HDR video, encoded for a video
>> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
>> pixel in the entire stream/file in nits.
>> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
>> average brightness in nits for a single frame. Max and Min Luminance
>> tells the max/min Luminance for the mastering display.
>> These values give an idea of the brightness of the video which can be
>> used by displays, so that they can adjust themselves for a better
>> viewing experience.
>>
> This does sound quite good for video players (and in the future image
> viewers), but might have something missing for HDR content generation
> (Blender, Krita, Natron, etc) since you do not always know these
> values in advance (it is effectively before mastering), most of these
> work in scene linear with the convention that 0.18 is middle grey
> (although this is just a convention). So I think that in these cases
> we might need to get info from the display system on what luminance
> levels its supports.
>
> Hope this makes sense since at the way to busy with other obligations
> so not much time to look into this.

Yes you are right, there would be cases where HDR content-does not have 
these values.
The HDR luminance levels for a display are exposed to the compositor 
through the edid from the kernel.
We can have a discussion whether a displays luminance level can be 
exposed by the compositor to the client and also how to do it.
But currently, the present patch is focused more for HDR video players, 
as it will be difficult to implement and
review all the scenarios/requirements at one go. Once the things set and 
agreed upon for this case,
we can add the support to expose these luminance values too as per 
discussions.
Does that make sense?

Regards,
Ankit

>> The protocol depends on the Color Management Protocol which is still
>> under review. There are couple of propsed protocols by Neils Ole [1],
>> and Sebastian Wick [2], which allow a client to select a color-space
>> for a surface, via ICC color profile files.
>> The client is expected to set the color space using the icc files and
>> the color-management protocol.
>>
>> [1] https://patchwork.freedesktop.org/patch/134570/
>> [2] https://patchwork.freedesktop.org/patch/286062/
>>
>> Co-authored-by: Harish Krupo <harish.krupo.kps@intel.com>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   Makefile.am                                        |  1 +
>>   unstable/hdr-mastering-metadata/README             |  5 ++
>>   .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
>>   3 files changed, 101 insertions(+)
>>   create mode 100644 unstable/hdr-mastering-metadata/README
>>   create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml  \
>>          $(NULL)
>>
>>   stable_protocols =                                                             \
>> diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
>> new file mode 100644
>> index 0000000..b567860
>> --- /dev/null
>> +++ b/unstable/hdr-mastering-metadata/README
>> @@ -0,0 +1,5 @@
>> +HDR-MASTERING-META-DATA-PROTOCOL
>> +
>> +Maintainers:
>> +Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> +Harish Krupo <harish.krupo.kps@intel.com>
>> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>> new file mode 100644
>> index 0000000..aeddf39
>> --- /dev/null
>> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>> @@ -0,0 +1,95 @@
>> +<?xml version="1.0" encoding="UTF-8"?>
>> +<protocol name="hdr_mastering_metadata_unstable_v1">
>> +
>> +  <copyright>
>> +    Copyright © 2019 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>
>> +
>> +  <description summary="hdr mastering meta data protocol">
>> +    This protocol provides the ability to specify the mastering color volume
>> +    metadata for an HDR video, for a given surface.
>> +    These values give an idea of the brightness of the video, which can be
>> +    used by the display so that it can adjust itself for a better viewing
>> +    experience.
>> +
>> +    The hdr-metadata values are enocoded in the video and the client can
>> +    retreive these values and provide them to the compositor.
>> +
>> +    A client need to first get the color-space interface for the HDR
>> +    color-space using the color-manager protocol, via ICC profile.
>> +    Then it needs to get the hdr_mastering_surface using
>> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
>> +    hdr_surface interface which can be used to set the hdr mastering meta-data.
>> +  </description>
>> +
>> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
>> +    <description summary="hdr mastering metadata">
>> +      The hdr matering metadata is a singleton global object that provides
>> +      the extenstion hdr_surface for a given surface.
>> +    </description>
>> +
>> +    <enum summary="error">
>> +      <entry name="hdr_surface_exists" value="0"
>> +             summary="The hdr surface exists for given surface."/>
>> +    </enum>
>> +
>> +    <request name="destroy" type="destructor">
>> +      <description summary="destroy the hdr_mastering_metadata object">
>> +        Destroy the HDR mastering metadata object.
>> +      </description>
>> +    </request>
>> +
>> +    <request name="get_hdr_surface">
>> +      <description summary="get the interface for hdr_surface">
>> +        This interface is created for a surface and the hdr mastering metadata
>> +       should be attached to this surface.
>> +      </description>
>> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
>> +      <arg name="surface" type="object" interface="wl_surface"/>
>> +    </request>
>> +
>> +  </interface>
>> +
>> +  <interface name="zwp_hdr_surface_v1" version="1">
>> +    <description summary="an interface to add hdr mastering metadata">
>> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
>> +      for a given surface.
>> +    </description>
>> +
>> +    <request name="set_hdr_mastering_metadata">
>> +      <description summary="set the hdr mastering metadata for the surface.">
>> +        This request is double buffered and it will be applied to the surface
>> +       on wl_surface::commit.
>> +      </description>
>> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
>> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
>> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
>> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>
>> +    </request>
>> +
>> +    <request name="destroy" type="destructor">
>> +      <description summary="destroy the hdr mastering surface object">
>> +        Destroy the hdr_surface.
>> +      </description>
>> +    </request>
>> +  </interface>
>> +</protocol>
>> --
>> 2.7.4
>>
On Wed, 27 Feb 2019 10:27:16 +0530
"Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:

> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> This protcol enables a client to send the hdr meta-data:
> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> SMPTE ST.2086.
> The clients get these values for an HDR video, encoded for a video
> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
> pixel in the entire stream/file in nits.
> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
> average brightness in nits for a single frame. Max and Min Luminance
> tells the max/min Luminance for the mastering display.
> These values give an idea of the brightness of the video which can be
> used by displays, so that they can adjust themselves for a better
> viewing experience.
> 
> The protocol depends on the Color Management Protocol which is still
> under review. There are couple of propsed protocols by Neils Ole [1],
> and Sebastian Wick [2], which allow a client to select a color-space
> for a surface, via ICC color profile files.
> The client is expected to set the color space using the icc files and
> the color-management protocol.
> 
> [1] https://patchwork.freedesktop.org/patch/134570/
> [2] https://patchwork.freedesktop.org/patch/286062/
> 
> Co-authored-by: Harish Krupo <harish.krupo.kps@intel.com>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Hi Ankit,

thanks for working on this, comments inline.

I do wonder if this should just be baked into a color management
extension directly. More on that below.

> ---
>  Makefile.am                                        |  1 +
>  unstable/hdr-mastering-metadata/README             |  5 ++
>  .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 unstable/hdr-mastering-metadata/README
>  create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> 
> diff --git a/Makefile.am b/Makefile.am
> index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml	\
>  	$(NULL)
>  
>  stable_protocols =								\
> diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
> new file mode 100644
> index 0000000..b567860
> --- /dev/null
> +++ b/unstable/hdr-mastering-metadata/README
> @@ -0,0 +1,5 @@
> +HDR-MASTERING-META-DATA-PROTOCOL
> +
> +Maintainers:
> +Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> +Harish Krupo <harish.krupo.kps@intel.com>
> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> new file mode 100644
> index 0000000..aeddf39
> --- /dev/null
> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml

Could it be named hdr-mastering rather than hdr-mastering-metadata?
Shorter C function names wouldn't hurt.

> @@ -0,0 +1,95 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<protocol name="hdr_mastering_metadata_unstable_v1">
> +
> +  <copyright>
> +    Copyright © 2019 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>
> +
> +  <description summary="hdr mastering meta data protocol">

I think this chapter should explicitly refer to the SMPTE
specification. You did it in the commit message, but I think it would be
appropriate here.

The commit message explains a lot of what this is. The commit message
should concentrate on why this extension is needed and why it is like
this, and leave the what for the protocol documentation.

> +    This protocol provides the ability to specify the mastering color volume
> +    metadata for an HDR video, for a given surface.
> +    These values give an idea of the brightness of the video, which can be
> +    used by the display so that it can adjust itself for a better viewing
> +    experience.
> +
> +    The hdr-metadata values are enocoded in the video and the client can
> +    retreive these values and provide them to the compositor.
> +
> +    A client need to first get the color-space interface for the HDR
> +    color-space using the color-manager protocol, via ICC profile.
> +    Then it needs to get the hdr_mastering_surface using
> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
> +    hdr_surface interface which can be used to set the hdr mastering meta-data.

Is this interface, or what it provides, completely useless without an
explicit color space / color image encoding definition?

Can one not apply the HDR parameters to the usual assumed sRGB content
with 8 bits per channel? Sure, it would probably look ugly, but so does
using a RGB332 pixel format for instance. I mean, mathematically the
definition of what should happen exists, right?

OTOH, if you do want to tie this to a color management extension, it
should probably use an object from the color management extension as
the base instead of wl_surface for instance. Or, you'd need an error
code for color management not set on the wl_surface.

Is there a case for compositors that implement color management but not
HDR support? Would it be unreasonable to make HDR parameters a part of
the color management extension? Such that it would be optional for a
client to set the HDR parameters.

What I mean by that is that color management already needs to be able
to handle out-of-gamut pixels somehow. Could out-of-dynamic-range be
handled with the exact same code, in case the compositor does not
support driving HDR monitors? Is there a significant implementation
effort to deal with the HDR parameters?

Erwin raised the issue of the client sometimes needing to know what the
outputs are capable of in terms of HDR. Can that include "not capable
of HDR" as well? Either as numerical values or special events. Probably
as special events, because you'd have to make random guesses on what
the HDR parameters of a SDR monitor are.

Knowing the capabilities of the outputs would also let video players
warn the user, if the played content exceeds their hardware
capabilities, or maybe even switch to a different video stream.

Btw. do HDR videos contain ICC profiles? Where would a client get the
ICC profile for a video it wants to show? Do we need to require the
compositor to expose some commonly used specific profiles?

> +  </description>
> +
> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
> +    <description summary="hdr mastering metadata">
> +      The hdr matering metadata is a singleton global object that provides
> +      the extenstion hdr_surface for a given surface.
> +    </description>
> +
> +    <enum summary="error">
> +      <entry name="hdr_surface_exists" value="0"
> +	      summary="The hdr surface exists for given surface."/>
> +    </enum>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the hdr_mastering_metadata object">
> +        Destroy the HDR mastering metadata object.

What side-effects does this have?

> +      </description>
> +    </request>
> +
> +    <request name="get_hdr_surface">
> +      <description summary="get the interface for hdr_surface">
> +        This interface is created for a surface and the hdr mastering metadata
> +	should be attached to this surface.
> +      </description>
> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
> +      <arg name="surface" type="object" interface="wl_surface"/>
> +    </request>
> +
> +  </interface>
> +
> +  <interface name="zwp_hdr_surface_v1" version="1">
> +    <description summary="an interface to add hdr mastering metadata">
> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
> +      for a given surface.

What happens if the wl_surface is destroyed first? And the client
issues requests through this interface?

> +    </description>
> +
> +    <request name="set_hdr_mastering_metadata">
> +      <description summary="set the hdr mastering metadata for the surface.">
> +        This request is double buffered and it will be applied to the surface
> +	on wl_surface::commit.

This is the request that actually turns HDR "on" for a wl_surface,
right?

Maybe there should be some words about SDR vs. HDR somewhere in the
spec, how they are handled differently.

> +      </description>
> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>

What are the units for all these values?

Do integers give enough precision and range?

Should these be split into separate requests? Could it be possible that
only some of these might be superseded by something else in the future?

OTOH, keeping all parameters in a single request means that there is no
error case of not defining all the parameters.

> +    </request>
> +
> +    <request name="destroy" type="destructor">
> +      <description summary="destroy the hdr mastering surface object">
> +        Destroy the hdr_surface.

What effects does this have to the wl_surface?

> +      </description>
> +    </request>
> +  </interface>
> +</protocol>


Thanks,
pq
Hi Pekka,

Thanks for the comments and suggestions.

Please my responses inline:

On 2/28/2019 7:51 PM, Pekka Paalanen wrote:
> On Wed, 27 Feb 2019 10:27:16 +0530
> "Nautiyal, Ankit K"<ankit.k.nautiyal@intel.com>  wrote:
>
>> From: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>>
>> This protcol enables a client to send the hdr meta-data:
>> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
>> SMPTE ST.2086.
>> The clients get these values for an HDR video, encoded for a video
>> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
>> pixel in the entire stream/file in nits.
>> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
>> average brightness in nits for a single frame. Max and Min Luminance
>> tells the max/min Luminance for the mastering display.
>> These values give an idea of the brightness of the video which can be
>> used by displays, so that they can adjust themselves for a better
>> viewing experience.
>>
>> The protocol depends on the Color Management Protocol which is still
>> under review. There are couple of propsed protocols by Neils Ole [1],
>> and Sebastian Wick [2], which allow a client to select a color-space
>> for a surface, via ICC color profile files.
>> The client is expected to set the color space using the icc files and
>> the color-management protocol.
>>
>> [1]https://patchwork.freedesktop.org/patch/134570/
>> [2]https://patchwork.freedesktop.org/patch/286062/
>>
>> Co-authored-by: Harish Krupo<harish.krupo.kps@intel.com>
>> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
> Hi Ankit,
>
> thanks for working on this, comments inline.
>
> I do wonder if this should just be baked into a color management
> extension directly. More on that below.

We had this in mind initially with this line of thought in couple of 
early interactions with Niels and Sebastian.
Later I had a discussion in #wayland with Sebastian, and it seemed that 
the brightness values should be handled separately,
as these do not directly come under ICC profiles as far as I understand.
As we know HDR  comprises of :
* Transfer functions
* Color primaries
* Mastering meta-data : MAX_CLL, MAX_FALL etc.
Out of these, the first two, can be handled by color-manager and would 
not change often.
HDR mastering meta-data, as is discussed is coming from the video 
stream, and with the dynamic HDR mastering metadata,
these brightness/luminance values might change frame by frame.
So it makes sense to have a separate protocol for this, where a client 
can tell the compositor about the color-space once with the 
color-manager protocol,
and these luminance values, can be sent to the compositor as per the 
video frames.

>> ---
>>   Makefile.am                                        |  1 +
>>   unstable/hdr-mastering-metadata/README             |  5 ++
>>   .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
>>   3 files changed, 101 insertions(+)
>>   create mode 100644 unstable/hdr-mastering-metadata/README
>>   create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml	\
>>   	$(NULL)
>>   
>>   stable_protocols =								\
>> diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
>> new file mode 100644
>> index 0000000..b567860
>> --- /dev/null
>> +++ b/unstable/hdr-mastering-metadata/README
>> @@ -0,0 +1,5 @@
>> +HDR-MASTERING-META-DATA-PROTOCOL
>> +
>> +Maintainers:
>> +Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>> +Harish Krupo<harish.krupo.kps@intel.com>
>> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>> new file mode 100644
>> index 0000000..aeddf39
>> --- /dev/null
>> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> Could it be named hdr-mastering rather than hdr-mastering-metadata?
> Shorter C function names wouldn't hurt.
Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ?

>> @@ -0,0 +1,95 @@
>> +<?xml version="1.0" encoding="UTF-8"?>
>> +<protocol name="hdr_mastering_metadata_unstable_v1">
>> +
>> +  <copyright>
>> +    Copyright © 2019 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>
>> +
>> +  <description summary="hdr mastering meta data protocol">
> I think this chapter should explicitly refer to the SMPTE
> specification. You did it in the commit message, but I think it would be
> appropriate here.
>
> The commit message explains a lot of what this is. The commit message
> should concentrate on why this extension is needed and why it is like
> this, and leave the what for the protocol documentation.

Point taken. Will make the commit concise and elaborate more in protocol 
documentation.


>> +    This protocol provides the ability to specify the mastering color volume
>> +    metadata for an HDR video, for a given surface.
>> +    These values give an idea of the brightness of the video, which can be
>> +    used by the display so that it can adjust itself for a better viewing
>> +    experience.
>> +
>> +    The hdr-metadata values are enocoded in the video and the client can
>> +    retreive these values and provide them to the compositor.
>> +
>> +    A client need to first get the color-space interface for the HDR
>> +    color-space using the color-manager protocol, via ICC profile.
>> +    Then it needs to get the hdr_mastering_surface using
>> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
>> +    hdr_surface interface which can be used to set the hdr mastering meta-data.
> Is this interface, or what it provides, completely useless without an
> explicit color space / color image encoding definition?
>
> Can one not apply the HDR parameters to the usual assumed sRGB content
> with 8 bits per channel? Sure, it would probably look ugly, but so does
> using a RGB332 pixel format for instance. I mean, mathematically the
> definition of what should happen exists, right?
You are absolutely right that these can be for other color space, and is 
not tied to HDR color space.
Had discussion with Harish from my team, who brought more clarity.

Harish, can you elaborate on this?

> OTOH, if you do want to tie this to a color management extension, it
> should probably use an object from the color management extension as
> the base instead of wl_surface for instance. Or, you'd need an error
> code for color management not set on the wl_surface.

Yes this probably needs more discussion.
Initially I was discussing the possibility of having the color-space 
interface as defined by Niel Ole's or Sebastian's proposed protocols.
As we understand, that these are not tied to HDR, but still HDR 
meta-data comprises of the color-primaries as well, so whether we
can use this protocol, without setting the color-space, I am not 
entirely sure.

> Is there a case for compositors that implement color management but not
> HDR support? Would it be unreasonable to make HDR parameters a part of
> the color management extension? Such that it would be optional for a
> client to set the HDR parameters.

As discussed above, seems best to have a separate protocol for these 
parameters.

> What I mean by that is that color management already needs to be able
> to handle out-of-gamut pixels somehow. Could out-of-dynamic-range be
> handled with the exact same code, in case the compositor does not
> support driving HDR monitors? Is there a significant implementation
> effort to deal with the HDR parameters?

I dont think that would be possible as I believe, there is significant 
implementation effort for HDR parameters
example tonemapping via Libva for HDR->SDR or SDR->HDR before blending.
Though some of the activities might be overlapping with color-management 
protocol implementation.
As per discussion with Sebastian on IRC, a separate protocol seemed a 
better.

> Erwin raised the issue of the client sometimes needing to know what the
> outputs are capable of in terms of HDR. Can that include "not capable
> of HDR" as well? Either as numerical values or special events. Probably
> as special events, because you'd have to make random guesses on what
> the HDR parameters of a SDR monitor are.
>
> Knowing the capabilities of the outputs would also let video players
> warn the user, if the played content exceeds their hardware
> capabilities, or maybe even switch to a different video stream.

Yes that makes sense, we can have an event from the compositor to the 
client, signifying that
the outputs on which the surface is visible, are "not HDR capable".
this information as far as I understand can be taken from edid of the 
displays,  at compositor side.

> Btw. do HDR videos contain ICC profiles? Where would a client get the
> ICC profile for a video it wants to show? Do we need to require the
> compositor to expose some commonly used specific profiles?

HDR videos do contain the color primaries, white point etc, which are 
covered in ICC profiles.
I guess both Niel's and Sebastian's solution provide a colorspace 
interface which can expose the built in specific profiles
to the client.
Whether these values, change during video playback, I am again not sure.
In that case perhaps we need to create an ICC profile from the values 
retrieved from the frame and provide to the compositor for the given 
surface.

>> +  </description>
>> +
>> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
>> +    <description summary="hdr mastering metadata">
>> +      The hdr matering metadata is a singleton global object that provides
>> +      the extenstion hdr_surface for a given surface.
>> +    </description>
>> +
>> +    <enum summary="error">
>> +      <entry name="hdr_surface_exists" value="0"
>> +	      summary="The hdr surface exists for given surface."/>
>> +    </enum>
>> +
>> +    <request name="destroy" type="destructor">
>> +      <description summary="destroy the hdr_mastering_metadata object">
>> +        Destroy the HDR mastering metadata object.
> What side-effects does this have?

When the client calls destroy, the hdr mastering metatadata object gets 
destroyed.
I believe the hdr_surface objects for that client should be destroyed as 
well.
What ever the surface/s the client have, will retain the last set values.
Client should call destroy only when it does not anticipate that it 
needs to send the value to client any more.
In case it needs to send the values again, it needs to bind again, which 
will be overhead I suppose.

>> +      </description>
>> +    </request>
>> +
>> +    <request name="get_hdr_surface">
>> +      <description summary="get the interface for hdr_surface">
>> +        This interface is created for a surface and the hdr mastering metadata
>> +	should be attached to this surface.
>> +      </description>
>> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
>> +      <arg name="surface" type="object" interface="wl_surface"/>
>> +    </request>
>> +
>> +  </interface>
>> +
>> +  <interface name="zwp_hdr_surface_v1" version="1">
>> +    <description summary="an interface to add hdr mastering metadata">
>> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
>> +      for a given surface.
> What happens if the wl_surface is destroyed first? And the client
> issues requests through this interface?

Valid point. The server should store the object for each surface as a 
list of surfaces, when the get_hdr_surface is called.
wl_surface must be checked, and error sent if the wl_surface is not there.

>> +    </description>
>> +
>> +    <request name="set_hdr_mastering_metadata">
>> +      <description summary="set the hdr mastering metadata for the surface.">
>> +        This request is double buffered and it will be applied to the surface
>> +	on wl_surface::commit.
> This is the request that actually turns HDR "on" for a wl_surface,
> right?
>
> Maybe there should be some words about SDR vs. HDR somewhere in the
> spec, how they are handled differently.

Yes thats correct, The idea is that while tone-mapping the color-spaces 
need to be convert to common space.
HDR->SDR or SDR->HDR.
Harish and Shashank from my team are working on it perhaps they can shed 
more light on this.

>> +      </description>
>> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
>> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
>> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
>> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>
> What are the units for all these values?
>
> Do integers give enough precision and range?

The values are in nits, AFAIK the values are of the order of hundreds 
and thousands(for max values)
So perhaps uint is sufficient.

> Should these be split into separate requests? Could it be possible that
> only some of these might be superseded by something else in the future?

Not sure about this, perhaps more parameters get added.

> OTOH, keeping all parameters in a single request means that there is no
> error case of not defining all the parameters.

>> +    </request>
>> +
>> +    <request name="destroy" type="destructor">
>> +      <description summary="destroy the hdr mastering surface object">
>> +        Destroy the hdr_surface.
> What effects does this have to the wl_surface?

Destroy the corresponding hdr_surface.
The client needs to call get_hdr_surface() again to change these values.
At wl_surface level, the luminance value should remain set to last value.

Regards,
Ankit

>> +      </description>
>> +    </request>
>> +  </interface>
>> +</protocol>
> Thanks,
> pq
Hi Pekka,

I have some comments to add to Ankit's. Please find them inline.

Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> writes:

> Hi Pekka,
>
> Thanks for the comments and suggestions.
>
> Please my responses inline:
>
> On 2/28/2019 7:51 PM, Pekka Paalanen wrote:
>> On Wed, 27 Feb 2019 10:27:16 +0530
>> "Nautiyal, Ankit K"<ankit.k.nautiyal@intel.com>  wrote:
>>
>>> From: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>>>
>>> This protcol enables a client to send the hdr meta-data:
>>> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
>>> SMPTE ST.2086.
>>> The clients get these values for an HDR video, encoded for a video
>>> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
>>> pixel in the entire stream/file in nits.
>>> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
>>> average brightness in nits for a single frame. Max and Min Luminance
>>> tells the max/min Luminance for the mastering display.
>>> These values give an idea of the brightness of the video which can be
>>> used by displays, so that they can adjust themselves for a better
>>> viewing experience.
>>>
>>> The protocol depends on the Color Management Protocol which is still
>>> under review. There are couple of propsed protocols by Neils Ole [1],
>>> and Sebastian Wick [2], which allow a client to select a color-space
>>> for a surface, via ICC color profile files.
>>> The client is expected to set the color space using the icc files and
>>> the color-management protocol.
>>>

I believe this part should be removed as the hdr_surface object wraps
over the wl_surface and doesn't interact with colorspace objects.
More on this below.

>>> [1]https://patchwork.freedesktop.org/patch/134570/
>>> [2]https://patchwork.freedesktop.org/patch/286062/
>>>
>>> Co-authored-by: Harish Krupo<harish.krupo.kps@intel.com>
>>> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>> Hi Ankit,
>>
>> thanks for working on this, comments inline.
>>
>> I do wonder if this should just be baked into a color management
>> extension directly. More on that below.
>
> We had this in mind initially with this line of thought in couple of early
> interactions with Niels and Sebastian.
> Later I had a discussion in #wayland with Sebastian, and it seemed that the
> brightness values should be handled separately,
> as these do not directly come under ICC profiles as far as I understand.
> As we know HDR  comprises of :
> * Transfer functions
> * Color primaries
> * Mastering meta-data : MAX_CLL, MAX_FALL etc.
> Out of these, the first two, can be handled by color-manager and would not
> change often.
> HDR mastering meta-data, as is discussed is coming from the video stream, and
> with the dynamic HDR mastering metadata,
> these brightness/luminance values might change frame by frame.
> So it makes sense to have a separate protocol for this, where a client can tell
> the compositor about the color-space once with the color-manager protocol,
> and these luminance values, can be sent to the compositor as per the video
> frames.
>

Agreed. The two protocols do not interact with each other and can be used
independently.

>>> ---
>>>   Makefile.am                                        |  1 +
>>>   unstable/hdr-mastering-metadata/README             |  5 ++
>>>   .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
>>>   3 files changed, 101 insertions(+)
>>>   create mode 100644 unstable/hdr-mastering-metadata/README
>>>   create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index 345ae6a..c097080 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-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml	\
>>>   	$(NULL)
>>>     stable_protocols =
>>> \
>>> diff --git a/unstable/hdr-mastering-metadata/README b/unstable/hdr-mastering-metadata/README
>>> new file mode 100644
>>> index 0000000..b567860
>>> --- /dev/null
>>> +++ b/unstable/hdr-mastering-metadata/README
>>> @@ -0,0 +1,5 @@
>>> +HDR-MASTERING-META-DATA-PROTOCOL
>>> +
>>> +Maintainers:
>>> +Ankit Nautiyal<ankit.k.nautiyal@intel.com>
>>> +Harish Krupo<harish.krupo.kps@intel.com>
>>> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>>> new file mode 100644
>>> index 0000000..aeddf39
>>> --- /dev/null
>>> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
>> Could it be named hdr-mastering rather than hdr-mastering-metadata?
>> Shorter C function names wouldn't hurt.
> Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ?
>
>>> @@ -0,0 +1,95 @@
>>> +<?xml version="1.0" encoding="UTF-8"?>
>>> +<protocol name="hdr_mastering_metadata_unstable_v1">
>>> +
>>> +  <copyright>
>>> +    Copyright © 2019 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>
>>> +
>>> +  <description summary="hdr mastering meta data protocol">
>> I think this chapter should explicitly refer to the SMPTE
>> specification. You did it in the commit message, but I think it would be
>> appropriate here.
>>
>> The commit message explains a lot of what this is. The commit message
>> should concentrate on why this extension is needed and why it is like
>> this, and leave the what for the protocol documentation.
>
> Point taken. Will make the commit concise and elaborate more in protocol
> documentation.
>
>
>>> +    This protocol provides the ability to specify the mastering color volume
>>> +    metadata for an HDR video, for a given surface.
>>> +    These values give an idea of the brightness of the video, which can be
>>> +    used by the display so that it can adjust itself for a better viewing
>>> +    experience.
>>> +
>>> +    The hdr-metadata values are enocoded in the video and the client can
>>> +    retreive these values and provide them to the compositor.
>>> +
>>> +    A client need to first get the color-space interface for the HDR
>>> +    color-space using the color-manager protocol, via ICC profile.
>>> +    Then it needs to get the hdr_mastering_surface using
>>> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
>>> +    hdr_surface interface which can be used to set the hdr mastering meta-data.
>> Is this interface, or what it provides, completely useless without an
>> explicit color space / color image encoding definition?
>>
>> Can one not apply the HDR parameters to the usual assumed sRGB content
>> with 8 bits per channel? Sure, it would probably look ugly, but so does
>> using a RGB332 pixel format for instance. I mean, mathematically the
>> definition of what should happen exists, right?
> You are absolutely right that these can be for other color space, and is not
> tied to HDR color space.
> Had discussion with Harish from my team, who brought more clarity.
>
> Harish, can you elaborate on this?
>

The two components, colorspace and HDR information are independent.
Although I haven't seen one but it is technically possible to have HDR
metadata associated with REC 709/sRGB image.

>> OTOH, if you do want to tie this to a color management extension, it
>> should probably use an object from the color management extension as
>> the base instead of wl_surface for instance. Or, you'd need an error
>> code for color management not set on the wl_surface.

We wouldn't need this as we will be using the wl_surface as our base
object.

>
> Yes this probably needs more discussion.
> Initially I was discussing the possibility of having the color-space interface
> as defined by Niel Ole's or Sebastian's proposed protocols.
> As we understand, that these are not tied to HDR, but still HDR meta-data
> comprises of the color-primaries as well, so whether we
> can use this protocol, without setting the color-space, I am not entirely sure.
>

The color primaries that are provided as a part of the HDR metadata are
the mastering display's primaries. It is possible that the primaries of
the mastering display did not exactly match with that of a standard
colorspace's (BT2020 / DCI-P3) primaries. Right now, for our use case
this information is sufficient. If we need that data in the future we
could add it as set_metadata_primaries and bump the protocol's version
to v2.

>> Is there a case for compositors that implement color management but not
>> HDR support? Would it be unreasonable to make HDR parameters a part of
>> the color management extension? Such that it would be optional for a
>> client to set the HDR parameters.

It is possible to just implement colospace conversion. The image quality
might not be very great if the luminance range in the image exceeds that
of the display. HDR handling will at least try to ensure that the
content's luminance matches that of the display.

>
> As discussed above, seems best to have a separate protocol for these parameters.
>
>> What I mean by that is that color management already needs to be able
>> to handle out-of-gamut pixels somehow. Could out-of-dynamic-range be
>> handled with the exact same code, in case the compositor does not
>> support driving HDR monitors? Is there a significant implementation
>> effort to deal with the HDR parameters?

Out-of-gamut pixels are handled completely separately compared to
out-of-dynamic-range pixels. Out of gamut can be solved either clamping
the color after conversion or by using a nearest approximation of the
color available within the colorspace.
Out-of-dynamic-range OTOH, requires applying tone mapping to adjust the
luminance ranges.

>
> I dont think that would be possible as I believe, there is significant
> implementation effort for HDR parameters
> example tonemapping via Libva for HDR->SDR or SDR->HDR before blending.
> Though some of the activities might be overlapping with color-management
> protocol implementation.
> As per discussion with Sebastian on IRC, a separate protocol seemed a better.
>
>> Erwin raised the issue of the client sometimes needing to know what the
>> outputs are capable of in terms of HDR. Can that include "not capable
>> of HDR" as well? Either as numerical values or special events. Probably
>> as special events, because you'd have to make random guesses on what
>> the HDR parameters of a SDR monitor are.
>>
>> Knowing the capabilities of the outputs would also let video players
>> warn the user, if the played content exceeds their hardware
>> capabilities, or maybe even switch to a different video stream.
>
> Yes that makes sense, we can have an event from the compositor to the client,
> signifying that
> the outputs on which the surface is visible, are "not HDR capable".
> this information as far as I understand can be taken from edid of the displays,
> at compositor side.
>

Yes, this indeed would be useful. Players like vlc implicitly convert
HDR videos to SDR before playing. If the player had information that the
display supports HDR, the player could skip the conversion and directly
present the buffer to the compositor. One way this could be implemented
is by adding an event in the HDR-METADATA interface to get this
information once the client binds to the interface. But this might not
be as simple as adding an event, as we would also need to ensure that
the client is always displayed on the same output/head which has the hdr
support.
Currently the scope of this protocol is to set information about the
surface not the other way around. Maybe we can discuss this once we
are done with one side of the communcation? :)

>> Btw. do HDR videos contain ICC profiles? Where would a client get the
>> ICC profile for a video it wants to show? Do we need to require the
>> compositor to expose some commonly used specific profiles?

HDR videos contain the primaries/luminance information of the display
which was used while mastering the content. FFMPEG lets us extract this
data per frame, by getting the frame's side data. More on this here [1].

>
> HDR videos do contain the color primaries, white point etc, which are covered in
> ICC profiles.
> I guess both Niel's and Sebastian's solution provide a colorspace interface
> which can expose the built in specific profiles
> to the client.
> Whether these values, change during video playback, I am again not sure.
> In that case perhaps we need to create an ICC profile from the values retrieved
> from the frame and provide to the compositor for the given surface.
>
>>> +  </description>
>>> +
>>> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
>>> +    <description summary="hdr mastering metadata">
>>> +      The hdr matering metadata is a singleton global object that provides
>>> +      the extenstion hdr_surface for a given surface.
>>> +    </description>
>>> +
>>> +    <enum summary="error">
>>> +      <entry name="hdr_surface_exists" value="0"
>>> +	      summary="The hdr surface exists for given surface."/>
>>> +    </enum>
>>> +
>>> +    <request name="destroy" type="destructor">
>>> +      <description summary="destroy the hdr_mastering_metadata object">
>>> +        Destroy the HDR mastering metadata object.
>> What side-effects does this have?
>
> When the client calls destroy, the hdr mastering metatadata object gets
> destroyed.
> I believe the hdr_surface objects for that client should be destroyed as well.
> What ever the surface/s the client have, will retain the last set values.
> Client should call destroy only when it does not anticipate that it needs to
> send the value to client any more.
> In case it needs to send the values again, it needs to bind again, which will be
> overhead I suppose.
>

Maybe not. When the client calls destroy for the the mastering metadata
object, the hdr_surface objects need not be destroyed as they dont
interact with the mastering metadata interface. The only obvious side
effect would be that the client will not be able to create more hdr_surface
objects.
Probably "release" would be a better request name than "destroy".

>>> +      </description>
>>> +    </request>
>>> +
>>> +    <request name="get_hdr_surface">
>>> +      <description summary="get the interface for hdr_surface">
>>> +        This interface is created for a surface and the hdr mastering metadata
>>> +	should be attached to this surface.
>>> +      </description>
>>> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
>>> +      <arg name="surface" type="object" interface="wl_surface"/>
>>> +    </request>
>>> +
>>> +  </interface>
>>> +
>>> +  <interface name="zwp_hdr_surface_v1" version="1">
>>> +    <description summary="an interface to add hdr mastering metadata">
>>> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
>>> +      for a given surface.
>> What happens if the wl_surface is destroyed first? And the client
>> issues requests through this interface?
>
> Valid point. The server should store the object for each surface as a list of
> surfaces, when the get_hdr_surface is called.
> wl_surface must be checked, and error sent if the wl_surface is not there.
>

The effect would be similar to what happens to viewport objects when the
base wl_surface is destroyed. This could be worded as:
     If the wl_surface associated with the hdr_surface is destroyed,
     all hdr_surface requests except 'destroy' raise the protocol error
     no_surface.

>>> +    </description>
>>> +
>>> +    <request name="set_hdr_mastering_metadata">
>>> +      <description summary="set the hdr mastering metadata for the surface.">
>>> +        This request is double buffered and it will be applied to the surface
>>> +	on wl_surface::commit.
>> This is the request that actually turns HDR "on" for a wl_surface,
>> right?
>>

Yes, it does.

>> Maybe there should be some words about SDR vs. HDR somewhere in the
>> spec, how they are handled differently.
>
> Yes thats correct, The idea is that while tone-mapping the color-spaces need to
> be convert to common space.
> HDR->SDR or SDR->HDR.
> Harish and Shashank from my team are working on it perhaps they can shed more
> light on this.
>

When HDR and SDR content are presented together, the compositor converts
all the buffers to HDR or SDR depending on the display's capability and
then composes the buffers. Would something in these lines be sufficient?

>>> +      </description>
>>> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
>>> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
>>> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
>>> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>
>> What are the units for all these values?
>>
>> Do integers give enough precision and range?
>
> The values are in nits, AFAIK the values are of the order of hundreds and
> thousands(for max values)
> So perhaps uint is sufficient.
>

The values are whole numbers and currently the maximum value is 10000
nits (max value that PQ curve encodes).

>> Should these be split into separate requests? Could it be possible that
>> only some of these might be superseded by something else in the future?
>
> Not sure about this, perhaps more parameters get added.
>
>> OTOH, keeping all parameters in a single request means that there is no
>> error case of not defining all the parameters.
>

Yes, this is a possibility. The compositor would need to pick "sane"
defaults for the values. It could pick the generally seen values for
max_lum and min_lum.

>>> +    </request>
>>> +
>>> +    <request name="destroy" type="destructor">
>>> +      <description summary="destroy the hdr mastering surface object">
>>> +        Destroy the hdr_surface.
>> What effects does this have to the wl_surface?
>
> Destroy the corresponding hdr_surface.
> The client needs to call get_hdr_surface() again to change these values.
> At wl_surface level, the luminance value should remain set to last value.
>

Once the object is destroyed, the associaed wl_surface would no more
have any HDR metadata. The buffer attached in the next commit would be
considered as a SDR buffer.

Thank you
Regards
Harish Krupo

[1] https://www.ffmpeg.org/doxygen/3.1/group__lavu__frame.html#gae01fa7e427274293aacdf2adc17076bc
On Fri, 01 Mar 2019 15:22:17 +0530
Harish Krupo <harish.krupo.kps@intel.com> wrote:

> Hi Pekka,
> 
> I have some comments to add to Ankit's. Please find them inline.
> 
> Nautiyal, Ankit K <ankit.k.nautiyal@intel.com> writes:

Hi Ankit and Harish,

comments below.


> > Hi Pekka,
> >
> > Thanks for the comments and suggestions.
> >
> > Please my responses inline:
> >
> > On 2/28/2019 7:51 PM, Pekka Paalanen wrote:  
> >> On Wed, 27 Feb 2019 10:27:16 +0530
> >> "Nautiyal, Ankit K"<ankit.k.nautiyal@intel.com>  wrote:
> >>  
> >>> From: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
> >>>
> >>> This protcol enables a client to send the hdr meta-data:
> >>> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> >>> SMPTE ST.2086.
> >>> The clients get these values for an HDR video, encoded for a video
> >>> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
> >>> pixel in the entire stream/file in nits.
> >>> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
> >>> average brightness in nits for a single frame. Max and Min Luminance
> >>> tells the max/min Luminance for the mastering display.
> >>> These values give an idea of the brightness of the video which can be
> >>> used by displays, so that they can adjust themselves for a better
> >>> viewing experience.
> >>>
> >>> The protocol depends on the Color Management Protocol which is still
> >>> under review. There are couple of propsed protocols by Neils Ole [1],
> >>> and Sebastian Wick [2], which allow a client to select a color-space
> >>> for a surface, via ICC color profile files.
> >>> The client is expected to set the color space using the icc files and
> >>> the color-management protocol.
> >>>  
> 
> I believe this part should be removed as the hdr_surface object wraps
> over the wl_surface and doesn't interact with colorspace objects.
> More on this below.
> 
> >>> [1]https://patchwork.freedesktop.org/patch/134570/
> >>> [2]https://patchwork.freedesktop.org/patch/286062/
> >>>
> >>> Co-authored-by: Harish Krupo<harish.krupo.kps@intel.com>
> >>> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>  
> >> Hi Ankit,
> >>
> >> thanks for working on this, comments inline.
> >>
> >> I do wonder if this should just be baked into a color management
> >> extension directly. More on that below.  
> >
> > We had this in mind initially with this line of thought in couple of early
> > interactions with Niels and Sebastian.
> > Later I had a discussion in #wayland with Sebastian, and it seemed that the
> > brightness values should be handled separately,
> > as these do not directly come under ICC profiles as far as I understand.
> > As we know HDR  comprises of :
> > * Transfer functions
> > * Color primaries
> > * Mastering meta-data : MAX_CLL, MAX_FALL etc.
> > Out of these, the first two, can be handled by color-manager and would not
> > change often.
> > HDR mastering meta-data, as is discussed is coming from the video stream, and
> > with the dynamic HDR mastering metadata,
> > these brightness/luminance values might change frame by frame.
> > So it makes sense to have a separate protocol for this, where a client can tell
> > the compositor about the color-space once with the color-manager protocol,
> > and these luminance values, can be sent to the compositor as per the video
> > frames.

We could have a separate request in the color management protocol
instead of having this whole another protocol extension, that is what I
meant. If ICC profile does not contain all the parameters HDR needs,
then add a request for those that were missed. Being a separate
request, a client can choose to send them or not.

The per-frame change is not a problem. Both color management proposals
already support changing the color profile for each frame. Not changing
the color profile while changing the HDR mastering parameters is not a
problem either.

The only question is, do color management and HDR support conceptually
make sense as independent features, or would implementations always
support both with essentially the same effort?

"Supporting HDR" here means only that the compositor is able to process
HDR content from clients, it does not include the capability to drive
HDR monitors. IOW, a compositor that only converts HDR content to SDR
is a valid HDR implementation from protocol perspective. It merely
advertises all outputs as SDR.

> 
> Agreed. The two protocols do not interact with each other and can be used
> independently.
> 
> >>> ---
> >>>   Makefile.am                                        |  1 +
> >>>   unstable/hdr-mastering-metadata/README             |  5 ++
> >>>   .../hdr-mastering-metadata-unstable-v1.xml         | 95 ++++++++++++++++++++++
> >>>   3 files changed, 101 insertions(+)
> >>>   create mode 100644 unstable/hdr-mastering-metadata/README
> >>>   create mode 100644 unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml

> >>> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> >>> new file mode 100644
> >>> index 0000000..aeddf39
> >>> --- /dev/null
> >>> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml  
> >> Could it be named hdr-mastering rather than hdr-mastering-metadata?
> >> Shorter C function names wouldn't hurt.  
> > Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ?
> >  
> >>> @@ -0,0 +1,95 @@
> >>> +<?xml version="1.0" encoding="UTF-8"?>
> >>> +<protocol name="hdr_mastering_metadata_unstable_v1">
> >>> +
> >>> +  <copyright>
> >>> +    Copyright © 2019 Intel
> >>> +

> >>> +  </copyright>
> >>> +
> >>> +  <description summary="hdr mastering meta data protocol">  
> >> I think this chapter should explicitly refer to the SMPTE
> >> specification. You did it in the commit message, but I think it would be
> >> appropriate here.
> >>
> >> The commit message explains a lot of what this is. The commit message
> >> should concentrate on why this extension is needed and why it is like
> >> this, and leave the what for the protocol documentation.  
> >
> > Point taken. Will make the commit concise and elaborate more in protocol
> > documentation.
> >
> >  
> >>> +    This protocol provides the ability to specify the mastering color volume
> >>> +    metadata for an HDR video, for a given surface.
> >>> +    These values give an idea of the brightness of the video, which can be
> >>> +    used by the display so that it can adjust itself for a better viewing
> >>> +    experience.
> >>> +
> >>> +    The hdr-metadata values are enocoded in the video and the client can
> >>> +    retreive these values and provide them to the compositor.
> >>> +
> >>> +    A client need to first get the color-space interface for the HDR
> >>> +    color-space using the color-manager protocol, via ICC profile.
> >>> +    Then it needs to get the hdr_mastering_surface using
> >>> +    hdr_mastering_metadata interface. The get_hdr_surface(), provides the
> >>> +    hdr_surface interface which can be used to set the hdr mastering meta-data.  
> >> Is this interface, or what it provides, completely useless without an
> >> explicit color space / color image encoding definition?
> >>
> >> Can one not apply the HDR parameters to the usual assumed sRGB content
> >> with 8 bits per channel? Sure, it would probably look ugly, but so does
> >> using a RGB332 pixel format for instance. I mean, mathematically the
> >> definition of what should happen exists, right?  
> > You are absolutely right that these can be for other color space, and is not
> > tied to HDR color space.
> > Had discussion with Harish from my team, who brought more clarity.
> >
> > Harish, can you elaborate on this?
> >  
> 
> The two components, colorspace and HDR information are independent.
> Although I haven't seen one but it is technically possible to have HDR
> metadata associated with REC 709/sRGB image.

Right, that means it makes sense to attach HDR mastering parameters to
a wl_surface, as opposed to a color image encoding.

> >> OTOH, if you do want to tie this to a color management extension, it
> >> should probably use an object from the color management extension as
> >> the base instead of wl_surface for instance. Or, you'd need an error
> >> code for color management not set on the wl_surface.  
> 
> We wouldn't need this as we will be using the wl_surface as our base
> object.

Sorry, that is cause and effect reversed.

If you do require color management (because it allows you to define the
color space for instance), then you should integrate with the color
management extension, not wl_surface.

If you do not require color management, then you can hook up to
wl_surface directly.


> >
> > Yes this probably needs more discussion.
> > Initially I was discussing the possibility of having the color-space interface
> > as defined by Niel Ole's or Sebastian's proposed protocols.
> > As we understand, that these are not tied to HDR, but still HDR meta-data
> > comprises of the color-primaries as well, so whether we
> > can use this protocol, without setting the color-space, I am not entirely sure.
> >  
> 
> The color primaries that are provided as a part of the HDR metadata are
> the mastering display's primaries. It is possible that the primaries of
> the mastering display did not exactly match with that of a standard
> colorspace's (BT2020 / DCI-P3) primaries. Right now, for our use case
> this information is sufficient. If we need that data in the future we
> could add it as set_metadata_primaries and bump the protocol's version
> to v2.

ICC profiles can define the primaries, that I am pretty sure of. If you
add another way to define primaries, you add confusion. Which extension
should win when both are used? Do you use anything else from the ICC
profile if you use the HDR primaries?

If you add the request to set the primaries, then I suggest you make
the HDR extension and color management extension mutually exclusive: a
wl_surface can only have one, never both.

The big question here is: does a HDR video, with reasonable defaults if
not explicitly defined, allow to programmatically manufacture a custom
ICC profile with the HDR primaries?

So far the color management extension proposals use ICC profiles as the
encoding of the color parameters. Either you leverage that or you
invent your own, everything in between is just a bog (or a bug).


> >> Is there a case for compositors that implement color management but not
> >> HDR support? Would it be unreasonable to make HDR parameters a part of
> >> the color management extension? Such that it would be optional for a
> >> client to set the HDR parameters.  
> 
> It is possible to just implement colospace conversion. The image quality
> might not be very great if the luminance range in the image exceeds that
> of the display. HDR handling will at least try to ensure that the
> content's luminance matches that of the display.

If a compositor implemented color management but not HDR (assuming the
two are separate), a client cannot even provide HDR content to begin
with, so I don't understand the comment about quality.

What I mean is, if a compositor implements color management, is there
any good reason to not implement HDR luminance mapping as well? At
least the implementation would not differ, just the parameters or
tables. That is why I suspect supporting HDR will be easy on top of a
good color conversion implementation.

Hence the question: is it worthwhile in protocol design to support
compositors that choose to implement color management but not HDR
luminance mapping?

It very well could be, which necessitates independent protocol
extensions.

> >
> > As discussed above, seems best to have a separate protocol for these parameters.
> >  
> >> What I mean by that is that color management already needs to be able
> >> to handle out-of-gamut pixels somehow. Could out-of-dynamic-range be
> >> handled with the exact same code, in case the compositor does not
> >> support driving HDR monitors? Is there a significant implementation
> >> effort to deal with the HDR parameters?  
> 
> Out-of-gamut pixels are handled completely separately compared to
> out-of-dynamic-range pixels. Out of gamut can be solved either clamping
> the color after conversion or by using a nearest approximation of the
> color available within the colorspace.
> Out-of-dynamic-range OTOH, requires applying tone mapping to adjust the
> luminance ranges.

I thought that some of the rendering intents of color management do the
same as HDR luminance mapping. Do they not? Only few parameters more to
adjust the ranges of the mapping curves.

> > I dont think that would be possible as I believe, there is significant
> > implementation effort for HDR parameters
> > example tonemapping via Libva for HDR->SDR or SDR->HDR before blending.
> > Though some of the activities might be overlapping with color-management
> > protocol implementation.
> > As per discussion with Sebastian on IRC, a separate protocol seemed a better.

Libva is a pure implementation detail, and I'm starting to think that
it would be better to not use it at all. If we have a capable color
conversion implementation for color management, it seems that HDR<->SDR
mapping would be just different parameter values to the same code.

We're going to need to do the conversion from client color image
encoding to the blending color space in a single pass, and we need to
do the conversion from blending space to output space in a single pass.
Can you do the conversions defined by arbitrary ICC profiles in libva?
I wouldn't expect so. Besides, the the former conversion is part of the
compositing, which I don't think is suitable for libva.

Tone mapping is not a separate step, it has to be built in to the color
space conversion shaders for memory bandwidth reasons.

I guess I should clarify that I am talking about the reference
implementation which we should aim for first. If there are special
cases where we can use libva to gain performance or save power, those
would come later.

> >  
> >> Erwin raised the issue of the client sometimes needing to know what the
> >> outputs are capable of in terms of HDR. Can that include "not capable
> >> of HDR" as well? Either as numerical values or special events. Probably
> >> as special events, because you'd have to make random guesses on what
> >> the HDR parameters of a SDR monitor are.
> >>
> >> Knowing the capabilities of the outputs would also let video players
> >> warn the user, if the played content exceeds their hardware
> >> capabilities, or maybe even switch to a different video stream.  
> >
> > Yes that makes sense, we can have an event from the compositor to the client,
> > signifying that
> > the outputs on which the surface is visible, are "not HDR capable".

That is a fairly awkward formulation. It would be better to have an
extension object created from a wl_output that then delivers the
per-output events describing the HDR capabilities of the output.
Clients already know which outputs are involved for a wl_surface, and
for color management there was the idea of additionally exposing which
output the client should optimize for.

It is also likely that color management will need a wl_output extension
object of its own, so if HDR information can be integrated in that, it
would be simpler to use.

> > this information as far as I understand can be taken from edid of the displays,
> > at compositor side.
> >  
> 
> Yes, this indeed would be useful. Players like vlc implicitly convert
> HDR videos to SDR before playing. If the player had information that the
> display supports HDR, the player could skip the conversion and directly
> present the buffer to the compositor. One way this could be implemented
> is by adding an event in the HDR-METADATA interface to get this
> information once the client binds to the interface. But this might not
> be as simple as adding an event, as we would also need to ensure that
> the client is always displayed on the same output/head which has the hdr
> support.

There is no need to ensure at the protocol level. If the compositor
advertises HDR support and there exists at least one HDR output, the
player should always produce HDR frames if possible, especially if it
is cheaper for the player than SDR. The compositor will take care of the
SDR conversion if necessary, and there won't be glitches if the window
moves between HDR and SDR monitors as there is no need for the client
to catch up.

> Currently the scope of this protocol is to set information about the
> surface not the other way around. Maybe we can discuss this once we
> are done with one side of the communcation? :)

I don't see any difficulty, we should include the output information
from the start too.

> >> Btw. do HDR videos contain ICC profiles? Where would a client get the
> >> ICC profile for a video it wants to show? Do we need to require the
> >> compositor to expose some commonly used specific profiles?  
> 
> HDR videos contain the primaries/luminance information of the display
> which was used while mastering the content. FFMPEG lets us extract this
> data per frame, by getting the frame's side data. More on this here [1].

Right. As I wrote earlier, if that is enough to manufacture a valid
custom ICC profile enough for the color conversions, it's good.

> >
> > HDR videos do contain the color primaries, white point etc, which are covered in
> > ICC profiles.
> > I guess both Niel's and Sebastian's solution provide a colorspace interface
> > which can expose the built in specific profiles
> > to the client.
> > Whether these values, change during video playback, I am again not sure.

Change is not a problem.

> > In that case perhaps we need to create an ICC profile from the values retrieved
> > from the frame and provide to the compositor for the given surface.
> >  
> >>> +  </description>
> >>> +
> >>> +  <interface name="zwp_hdr_mastering_metadata_v1" version="1">
> >>> +    <description summary="hdr mastering metadata">
> >>> +      The hdr matering metadata is a singleton global object that provides
> >>> +      the extenstion hdr_surface for a given surface.
> >>> +    </description>
> >>> +
> >>> +    <enum summary="error">
> >>> +      <entry name="hdr_surface_exists" value="0"
> >>> +	      summary="The hdr surface exists for given surface."/>
> >>> +    </enum>
> >>> +
> >>> +    <request name="destroy" type="destructor">
> >>> +      <description summary="destroy the hdr_mastering_metadata object">
> >>> +        Destroy the HDR mastering metadata object.  
> >> What side-effects does this have?  
> >
> > When the client calls destroy, the hdr mastering metatadata object gets
> > destroyed.
> > I believe the hdr_surface objects for that client should be destroyed as well.
> > What ever the surface/s the client have, will retain the last set values.
> > Client should call destroy only when it does not anticipate that it needs to
> > send the value to client any more.
> > In case it needs to send the values again, it needs to bind again, which will be
> > overhead I suppose.
> >  
> 
> Maybe not. When the client calls destroy for the the mastering metadata
> object, the hdr_surface objects need not be destroyed as they dont
> interact with the mastering metadata interface. The only obvious side
> effect would be that the client will not be able to create more hdr_surface
> objects.

Yes, exactly as Harish says. It's a common convention that the child
objects already created are not affected. Anything else would be just
more error cases.

> Probably "release" would be a better request name than "destroy".

No, "destroy" is the canonical name.

The only reason why some interfaces have "release" is that it was added
after the fact. Originally the interface did not have a destructor at
all, then we found out that we need a destructor for the compositor to
be able to free resources while the client is still connected. We added
the destructor, but we could not call it "destroy" because
wayland-scanner generates a *_destroy() function in any case, and we
must not change the behaviour of it. So the destructor was named
"release", to ever remind us of our mistake.

> 
> >>> +      </description>
> >>> +    </request>
> >>> +
> >>> +    <request name="get_hdr_surface">
> >>> +      <description summary="get the interface for hdr_surface">
> >>> +        This interface is created for a surface and the hdr mastering metadata
> >>> +	should be attached to this surface.
> >>> +      </description>
> >>> +      <arg name="hdr_surface" type="new_id" interface="zwp_hdr_surface_v1"/>
> >>> +      <arg name="surface" type="object" interface="wl_surface"/>
> >>> +    </request>
> >>> +
> >>> +  </interface>
> >>> +
> >>> +  <interface name="zwp_hdr_surface_v1" version="1">
> >>> +    <description summary="an interface to add hdr mastering metadata">
> >>> +      An interface to add the hdr mastering metadata like MAX-CLL and MAX-FALL,
> >>> +      for a given surface.  
> >> What happens if the wl_surface is destroyed first? And the client
> >> issues requests through this interface?  
> >
> > Valid point. The server should store the object for each surface as a list of
> > surfaces, when the get_hdr_surface is called.
> > wl_surface must be checked, and error sent if the wl_surface is not there.

That sounds... complicated. We have plenty of examples, and almost all
just have one pointer to set to NULL from a wl_surface destroy listener.

> 
> The effect would be similar to what happens to viewport objects when the
> base wl_surface is destroyed. This could be worded as:
>      If the wl_surface associated with the hdr_surface is destroyed,
>      all hdr_surface requests except 'destroy' raise the protocol error
>      no_surface.

Yes, I would prefer that.

> 
> >>> +    </description>
> >>> +
> >>> +    <request name="set_hdr_mastering_metadata">
> >>> +      <description summary="set the hdr mastering metadata for the surface.">
> >>> +        This request is double buffered and it will be applied to the surface
> >>> +	on wl_surface::commit.  
> >> This is the request that actually turns HDR "on" for a wl_surface,
> >> right?
> >>  
> 
> Yes, it does.
> 
> >> Maybe there should be some words about SDR vs. HDR somewhere in the
> >> spec, how they are handled differently.  
> >
> > Yes thats correct, The idea is that while tone-mapping the color-spaces need to
> > be convert to common space.
> > HDR->SDR or SDR->HDR.
> > Harish and Shashank from my team are working on it perhaps they can shed more
> > light on this.
> >  
> 
> When HDR and SDR content are presented together, the compositor converts
> all the buffers to HDR or SDR depending on the display's capability and
> then composes the buffers. Would something in these lines be sufficient?

I was thinking of more along the lines:
- a short definition of what SDR and HDR mean
- that a surface is SDR by default
- how to return a surface to SDR after it was set HDR (hdr_surface destructor)

> >>> +      </description>
> >>> +      <arg name="max_cll" type="uint" summary="MAX Content Light Level"/>
> >>> +      <arg name="max_fall" type="uint" summary="MAX Frame Average Light Level"/>
> >>> +      <arg name="max_lum" type="uint" summary="MAX Luminance"/>
> >>> +      <arg name="min_lum" type="uint" summary="MIN Luminance"/>  
> >> What are the units for all these values?
> >>
> >> Do integers give enough precision and range?  
> >
> > The values are in nits, AFAIK the values are of the order of hundreds and
> > thousands(for max values)
> > So perhaps uint is sufficient.
> >  
> 
> The values are whole numbers and currently the maximum value is 10000
> nits (max value that PQ curve encodes).

Sounds fine.

Is it good enough for min luminance as well? What are typical min_lum
values? Is there no need for values between 0 and 1? Or more precision
at the low end?

> >> Should these be split into separate requests? Could it be possible that
> >> only some of these might be superseded by something else in the future?  
> >
> > Not sure about this, perhaps more parameters get added.
> >  
> >> OTOH, keeping all parameters in a single request means that there is no
> >> error case of not defining all the parameters.  
> >  
> 
> Yes, this is a possibility. The compositor would need to pick "sane"
> defaults for the values. It could pick the generally seen values for
> max_lum and min_lum.

I'm a little wary of default values.

An SDR monitor does not tell us these values at all. If default values
were picked, we should use them consistently with both surface defaults
and output defaults. If a client explicitly sets values that are
the defaults, then the result on screen should be the same as if it
never set them. OTOH, if a compositor describes an output with the
default values, a client might get confused if the output is SDR or HDR.

Considering all that, should there be default values, or should there
be an explicit and unique "flag" for SDR? E.g. lack of making a request
or receiving an event would imply SDR.

> >>> +    </request>
> >>> +
> >>> +    <request name="destroy" type="destructor">
> >>> +      <description summary="destroy the hdr mastering surface object">
> >>> +        Destroy the hdr_surface.  
> >> What effects does this have to the wl_surface?  
> >
> > Destroy the corresponding hdr_surface.
> > The client needs to call get_hdr_surface() again to change these values.
> > At wl_surface level, the luminance value should remain set to last value.
> >  
> 
> Once the object is destroyed, the associaed wl_surface would no more
> have any HDR metadata. The buffer attached in the next commit would be
> considered as a SDR buffer.

Yes, Harish made a good definition. I'd even drop the mention of a new
buffer and just rely on the double-buffered state, otherwise you have
to define what happens if the next commit does not have a new buffer
attached.


Thanks,
pq


> 
> Thank you
> Regards
> Harish Krupo
> 
> [1] https://www.ffmpeg.org/doxygen/3.1/group__lavu__frame.html#gae01fa7e427274293aacdf2adc17076bc
On Fri, 1 Mar 2019 14:40:16 +0530
"Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:

> Hi Pekka,
> 
> Thanks for the comments and suggestions.
> 
> Please my responses inline:
> 
> On 2/28/2019 7:51 PM, Pekka Paalanen wrote:
> > On Wed, 27 Feb 2019 10:27:16 +0530
> > "Nautiyal, Ankit K"<ankit.k.nautiyal@intel.com>  wrote:
> >  
> >> From: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
> >>
> >> This protcol enables a client to send the hdr meta-data:
> >> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> >> SMPTE ST.2086.
> >> The clients get these values for an HDR video, encoded for a video
> >> stream/file. MAX-CLL (Maximum Content Light Level) tells the brightest
> >> pixel in the entire stream/file in nits.
> >> MAX-FALL (Maximum Frame Average Light Level) tells the highest frame
> >> average brightness in nits for a single frame. Max and Min Luminance
> >> tells the max/min Luminance for the mastering display.
> >> These values give an idea of the brightness of the video which can be
> >> used by displays, so that they can adjust themselves for a better
> >> viewing experience.
> >>
> >> The protocol depends on the Color Management Protocol which is still
> >> under review. There are couple of propsed protocols by Neils Ole [1],
> >> and Sebastian Wick [2], which allow a client to select a color-space
> >> for a surface, via ICC color profile files.
> >> The client is expected to set the color space using the icc files and
> >> the color-management protocol.
> >>
> >> [1]https://patchwork.freedesktop.org/patch/134570/
> >> [2]https://patchwork.freedesktop.org/patch/286062/
> >>
> >> Co-authored-by: Harish Krupo<harish.krupo.kps@intel.com>
> >> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>  

> >> diff --git a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml
> >> new file mode 100644
> >> index 0000000..aeddf39
> >> --- /dev/null
> >> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v1.xml  
> > Could it be named hdr-mastering rather than hdr-mastering-metadata?
> > Shorter C function names wouldn't hurt.  
> Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ?

Hi Ankit,

to me, the word "metadata" is the redundant one. "HDR" is significant,
"mastering" is probably significant as well, but since we are talking
about Wayland, half of the protocol is essentially metadata.

But "hdr-metadata" is not bad either.

Then we have hdr_surface.set_hdr_mastering_metadata. That could be
simply hdr_surface.set_metadata... or hdr_surface.set_luminance since
all the arguments are about luminance somehow.

Does mastering metadata include more than just luminance parameters?
The primaries and white point maybe?


Thanks,
pq
> From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> Sent: Friday, March 1, 2019 7:49 PM
> To: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>
> Cc: wayland-devel@lists.freedesktop.org; sebastian@sebastianwick.net;
> Sharma, Shashank <shashank.sharma@intel.com>; e.burema@gmail.com;
> niels_ole@salscheider-online.de; Kps, Harish Krupo
> <harish.krupo.kps@intel.com>
> Subject: Re: [PATCH] unstable: add HDR Mastering Meta-data Protocol
> 
> On Fri, 1 Mar 2019 14:40:16 +0530
> "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> 
> > Hi Pekka,
> >
> > Thanks for the comments and suggestions.
> >
> > Please my responses inline:
> >
> > On 2/28/2019 7:51 PM, Pekka Paalanen wrote:
> > > On Wed, 27 Feb 2019 10:27:16 +0530
> > > "Nautiyal, Ankit K"<ankit.k.nautiyal@intel.com>  wrote:
> > >
> > >> From: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
> > >>
> > >> This protcol enables a client to send the hdr meta-data:
> > >> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> > >> SMPTE ST.2086.
> > >> The clients get these values for an HDR video, encoded for a video
> > >> stream/file. MAX-CLL (Maximum Content Light Level) tells the
> > >> brightest pixel in the entire stream/file in nits.
> > >> MAX-FALL (Maximum Frame Average Light Level) tells the highest
> > >> frame average brightness in nits for a single frame. Max and Min
> > >> Luminance tells the max/min Luminance for the mastering display.
> > >> These values give an idea of the brightness of the video which can
> > >> be used by displays, so that they can adjust themselves for a
> > >> better viewing experience.
> > >>
> > >> The protocol depends on the Color Management Protocol which is
> > >> still under review. There are couple of propsed protocols by Neils
> > >> Ole [1], and Sebastian Wick [2], which allow a client to select a
> > >> color-space for a surface, via ICC color profile files.
> > >> The client is expected to set the color space using the icc files
> > >> and the color-management protocol.
> > >>
> > >> [1]https://patchwork.freedesktop.org/patch/134570/
> > >> [2]https://patchwork.freedesktop.org/patch/286062/
> > >>
> > >> Co-authored-by: Harish Krupo<harish.krupo.kps@intel.com>
> > >> Signed-off-by: Ankit Nautiyal<ankit.k.nautiyal@intel.com>
> 
> > >> diff --git
> > >> a/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v
> > >> 1.xml
> > >> b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstable-v
> > >> 1.xml
> > >> new file mode 100644
> > >> index 0000000..aeddf39
> > >> --- /dev/null
> > >> +++ b/unstable/hdr-mastering-metadata/hdr-mastering-metadata-unstab
> > >> +++ le-v1.xml
> > > Could it be named hdr-mastering rather than hdr-mastering-metadata?
> > > Shorter C function names wouldn't hurt.
> > Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ?
> 
> Hi Ankit,
> 
> to me, the word "metadata" is the redundant one. "HDR" is significant,
> "mastering" is probably significant as well, but since we are talking about
> Wayland, half of the protocol is essentially metadata.
> 
> But "hdr-metadata" is not bad either.
> 
> Then we have hdr_surface.set_hdr_mastering_metadata. That could be simply
> hdr_surface.set_metadata... or hdr_surface.set_luminance since all the
> arguments are about luminance somehow.
> 
> Does mastering metadata include more than just luminance parameters?
> The primaries and white point maybe?
> 

Yes, it does. It contains the primaries and white point along with luminance 
of the display used for mastering the content.
Pekka Paalanen wrote:
> On Wed, 27 Feb 2019 10:27:16 +0530
> "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> 
>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>
>> This protcol enables a client to send the hdr meta-data:
>> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
>> SMPTE ST.2086.

Hmm. I'm wondering what the intent of this is.

If it is to feed these values through to the display
itself so that it can do dynamic HDR rendering, then
this tends to cut across color management, since
color management as it is currently formulated depends
on reproducability in color devices. If really desired,
I guess this would have to be some kind of special mode that
bypasses the usual rendering pipeline.

If the idea is that somehow the compositor is to do
dynamic HDR rendering, then I have my doubts about its
suitability and practicality, unless you are prepared to
spell out in detail the algorithm it should use for performing this.

My assumption was that the compositor would only be expected
to do well understood fallback color conversions, and that
anything that needed special functionality or that was
going to do experimental color handling (such as
dynamic HDR rendering) would do so in the client.

For the purposes of mixing SDR and HDR in the compositor
as a fallback, a much simpler set of transforms should be sufficient,
namely a brightness for converting SDR into HDR, and a tone mapping
curve or equivalent settings for compressing HDR into SDR.

> Is there a case for compositors that implement color management but not
> HDR support? Would it be unreasonable to make HDR parameters a part of
> the color management extension? Such that it would be optional for a
> client to set the HDR parameters.

I think that at this point in time it is entirely reasonable
that a compositing system have some level of HDR awareness.

> Btw. do HDR videos contain ICC profiles? Where would a client get the
> ICC profile for a video it wants to show? Do we need to require the
> compositor to expose some commonly used specific profiles?

As I understand it, no they don't. They either rely on a stated
encoding (i.e. REC2020 based), or sometimes have simple meta-data
such as primary coordinates & transfer curve specifications. A client
wanting to support such meta-data can simply create a matching ICC
profile on the fly if it wishes.

Cheers,
	Graeme Gill.
Regards
Shashank

> -----Original Message-----
> From: Graeme Gill [mailto:graeme2@argyllcms.com]
> Sent: Tuesday, March 5, 2019 9:35 AM
> To: Pekka Paalanen <ppaalanen@gmail.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>
> Cc: e.burema@gmail.com; niels_ole@salscheider-online.de; wayland-
> devel@lists.freedesktop.org; sebastian@sebastianwick.net; Kps, Harish Krupo
> <harish.krupo.kps@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
> Subject: Re: [PATCH] unstable: add HDR Mastering Meta-data Protocol
> 
> Pekka Paalanen wrote:
> > On Wed, 27 Feb 2019 10:27:16 +0530
> > "Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com> wrote:
> >
> >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>
> >> This protcol enables a client to send the hdr meta-data:
> >> MAX-CLL, MAX-FALL, Max Luminance and Min Luminance as defined by
> >> SMPTE ST.2086.
> 
> Hmm. I'm wondering what the intent of this is.
> 
> If it is to feed these values through to the display itself so that it can do dynamic HDR
> rendering, then this tends to cut across color management, since color management
> as it is currently formulated depends on reproducability in color devices. If really
> desired, I guess this would have to be some kind of special mode that bypasses the
> usual rendering pipeline.
> 
The main reason is to pass it to the compositor so that it can do the tone mapping properly. As you know, there could be cases where we are dealing with one HDR and multiple SDR frames. Now, the HDR content comes with its own metadata information, which needs to be passed to the display using the AVI infoframes, for the correct HDR experience, but we need to make others non-HDR frames also compatible to this brightness range of metadata, so we do tone mapping. 

We are writing compositor code, which will take a call on target outputs content brightness level too (apart with colorspace) by applying appropriate tone mapping (SDR to HDR, HDR to HDR, or HDR to SDR)  using libVA or GL extensions. This will make sure that everything being shown on the display falls into same brightness range. 
 
> If the idea is that somehow the compositor is to do dynamic HDR rendering, then I
> have my doubts about its suitability and practicality, unless you are prepared to spell
> out in detail the algorithm it should use for performing this.
> 
The GL tone mapping algorithms will be opensource methods, and will be available for the code review. HW tone mapping is using the libVA interface and media-driver handler.  
> My assumption was that the compositor would only be expected to do well
> understood fallback color conversions, and that anything that needed special
> functionality or that was going to do experimental color handling (such as dynamic
> HDR rendering) would do so in the client.
> 
Yes, we are trying our best here to make compositor fair and intelligent, and I believe with proper discussions and code reviews we should be able to reach that level. 
> For the purposes of mixing SDR and HDR in the compositor as a fallback, a much
> simpler set of transforms should be sufficient, namely a brightness for converting
> SDR into HDR, and a tone mapping curve or equivalent settings for compressing HDR
> into SDR.
> 
Agree. 
> > Is there a case for compositors that implement color management but
> > not HDR support? Would it be unreasonable to make HDR parameters a
> > part of the color management extension? Such that it would be optional
> > for a client to set the HDR parameters.
> 
Right now we are not targeting this, but eventually we want to be there. As we all know, it's a huge area to cover in an single attempt, and gets very cumbersome. So the plan is to first implement a modular limited capability HDR video playback, and  use that as a driving and testing vehicle for color management, and then later enhance it with adding more and more modules.  
> I think that at this point in time it is entirely reasonable that a compositing system have
> some level of HDR awareness.
> 
Yes, the idea is to make it completely aware of HDR scenarios over the period, over a slowly maturing stack.  

- Shashank
> > Btw. do HDR videos contain ICC profiles? Where would a client get the
> > ICC profile for a video it wants to show? Do we need to require the
> > compositor to expose some commonly used specific profiles?
> 
> As I understand it, no they don't. They either rely on a stated encoding (i.e. REC2020
> based), or sometimes have simple meta-data such as primary coordinates & transfer
> curve specifications. A client wanting to support such meta-data can simply create a
> matching ICC profile on the fly if it wishes.
> 
> Cheers,
> 	Graeme Gill.
Pekka Paalanen wrote:

> The only question is, do color management and HDR support conceptually
> make sense as independent features, or would implementations always
> support both with essentially the same effort?

In my view, you can certainly add an HDR extension independently
of a color management extension, but it would either be a hack
to get things working (i.e. make all sorts of general display colorspace
assumptions or approximations) or if less hacky, would start to encompass
general color management concerns. A Color Management extension on the
other hand adds a bunch of non-HDR related stuff, but HDR then slots in as
a sub-case of handling differences in display characteristics.

> "Supporting HDR" here means only that the compositor is able to process
> HDR content from clients, it does not include the capability to drive
> HDR monitors. IOW, a compositor that only converts HDR content to SDR
> is a valid HDR implementation from protocol perspective. It merely
> advertises all outputs as SDR.

True, but once you allow for a display to be labelled HDR and
have an associated color profile, there isn't that much to handling HDR
like any other display.

> The big question here is: does a HDR video, with reasonable defaults if
> not explicitly defined, allow to programmatically manufacture a custom
> ICC profile with the HDR primaries?

Yes, this works. Some HDR TV calibration already takes this approach.

> What I mean is, if a compositor implements color management, is there
> any good reason to not implement HDR luminance mapping as well? At
> least the implementation would not differ, just the parameters or
> tables. That is why I suspect supporting HDR will be easy on top of a
> good color conversion implementation.

I agree.

> Harish Krupo <harish.krupo.kps@intel.com> wrote:

>> Out-of-gamut pixels are handled completely separately compared to
>> out-of-dynamic-range pixels. Out of gamut can be solved either clamping
>> the color after conversion or by using a nearest approximation of the
>> color available within the colorspace.
>> Out-of-dynamic-range OTOH, requires applying tone mapping to adjust the
>> luminance ranges.

Adapting the luminance range is just another aspect of mapping
one color space to another. One mechanism for achieving this is
to use a relative colorspace description of the spaces and then
use an orthogonal luminance mapping (such as tone mapping for HDR -> SDR).

> I thought that some of the rendering intents of color management do the
> same as HDR luminance mapping. Do they not? Only few parameters more to
> adjust the ranges of the mapping curves.

Standard ICC intents generally do not, because most of them work
around a Relative Colorimetric assumption. Absolute Colorimetric
in theory could support this, but in practice is not a mechanism
that would work the way you want for HDR handling (i.e.
it would work for SDR->HDR, but would clip HDR->SDR, and
would have the side effect of not aligning the white points,
which you probably don't want it to do.)

But I think it should be relatively straight forward to
augment the standard ICC CMM linking parameters with some
HDR options. (In terms of implementation I would hope
that lcms's plugin architecture would help facilitate this,
or at worst it could be patches and the changes fed back
upstream to Marti). The two options I would imagine
adding would be SDR->HDR brightness (i.e. what the
SDR white brightness should be), and for HDR->SDR what
the HDR diffuse white is and possibly a tone mapping
strategy. (The technical details of this can be guided
by something like the ITU-R BT.2390-2 EETF).

> It is also likely that color management will need a wl_output extension
> object of its own, so if HDR information can be integrated in that, it
> would be simpler to use.

Yep.

> There is no need to ensure at the protocol level. If the compositor
> advertises HDR support and there exists at least one HDR output, the
> player should always produce HDR frames if possible, especially if it
> is cheaper for the player than SDR. The compositor will take care of the
> SDR conversion if necessary, and there won't be glitches if the window
> moves between HDR and SDR monitors as there is no need for the client
> to catch up.

In a color managed implementation, the player just need mark the
source with the video ICC profile and HDR intent information for
default handling by the Compositor. If it wanted to do
the HDR handling itself, it would download the output ICC profile
to figure out its conversion from video source to output space,
and then mark the buffer as being in the primary output colorspace
so that the Compositor knows it doesn't have to do anything.

Cheers,
	Graeme Gill.
Sharma, Shashank wrote:

>> From: Graeme Gill [mailto:graeme2@argyllcms.com]

>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> 
>>>> This protcol enables a client to send the hdr meta-data: MAX-CLL, MAX-FALL, Max
>>>> Luminance and Min Luminance as defined by SMPTE ST.2086.
>> 
>> Hmm. I'm wondering what the intent of this is.

> The main reason is to pass it to the compositor so that it can do the tone mapping
> properly. As you know, there could be cases where we are dealing with one HDR and
> multiple SDR frames. Now, the HDR content comes with its own metadata information,
> which needs to be passed to the display using the AVI infoframes, for the correct HDR
> experience, but we need to make others non-HDR frames also compatible to this
> brightness range of metadata, so we do tone mapping.

I've been thinking a bit about that, and currently my view is
that this is not the best way of arranging things. MAX-CLL and MAX-FALL
are specific to certain current source of HD imagery, and not all HDR
sources will have those specific numbers, and video standards may change
in the future (i.e. frame by frame meta-data). Exactly how they can be
used is unclear - i.e. they are observations from a video stream, not
parameters for a specific tone mapping. So punting them to the Compositor
doesn't seem such a good approach.

Instead what I'd suggest is providing some concrete tone mapping
operators for HDR handling (at least for V1), with well defined behavior
and parameters. The video playback application can do the conversion
from source format specific observations like MAX-CLL and MAX-FALL
into the specific compositor tone mapping parameters, or it has the option of
determining those parameters some other way (user setting perhaps,
or it's own analysis of the video stream), or it can
even do the adaptation of the source to the display itself,
since it has access to the display characteristics via the ICC profile.

I think it's quite possible to have the Compositor do sane HDR conversions
with no other extra information than the HDR nominal diffuse white
brightness for each HDR profile, and perhaps this could be a base Compositor
tone mapping behavior, with other tone mapping algorithms (and
their associated parameters) added as they are identified and
defined or standardized.

>> If the idea is that somehow the compositor is to do dynamic HDR rendering, then I 
>> have my doubts about its suitability and practicality, unless you are prepared to
>> spell out in detail the algorithm it should use for performing this.
>> 
> The GL tone mapping algorithms will be opensource methods, and will be available for
> the code review. HW tone mapping is using the libVA interface and media-driver handler.

Right, but there is a distinction between a particular implementation and
a Compositor specification.

Can you write a specification of how the tone curves you are proposing
to use work, in such a way that someone else can implement them, and
that someone dealing with other HDR sources can use them ?
(i.e. HDR photographs or synthetic imagery from games etc.)

> Right now we are not targeting this, but eventually we want to be there. As we all
> know, it's a huge area to cover in an single attempt, and gets very cumbersome. So the
> plan is to first implement a modular limited capability HDR video playback, and  use
> that as a driving and testing vehicle for color management, and then later enhance it
> with adding more and more modules.

Sure. I'm a little worried though, that video specific HDR considerations will
end up getting set in stone, and hamper more general HDR compatibility.

Cheers,

Graeme Gill.
Regards
Shashank

> -----Original Message-----
> From: Graeme Gill [mailto:graeme2@argyllcms.com]
> Sent: Thursday, March 7, 2019 7:05 AM
> To: Sharma, Shashank <shashank.sharma@intel.com>; graeme@argyllcms.com;
> Pekka Paalanen <ppaalanen@gmail.com>; Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com>
> Cc: e.burema@gmail.com; Kps, Harish Krupo <harish.krupo.kps@intel.com>;
> niels_ole@salscheider-online.de; sebastian@sebastianwick.net; wayland-
> devel@lists.freedesktop.org
> Subject: Re: [PATCH] unstable: add HDR Mastering Meta-data Protocol
> 
> Sharma, Shashank wrote:
> 
> >> From: Graeme Gill [mailto:graeme2@argyllcms.com]
> 
> >>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>>
> >>>> This protcol enables a client to send the hdr meta-data: MAX-CLL,
> >>>> MAX-FALL, Max Luminance and Min Luminance as defined by SMPTE ST.2086.
> >>
> >> Hmm. I'm wondering what the intent of this is.
> 
> > The main reason is to pass it to the compositor so that it can do the
> > tone mapping properly. As you know, there could be cases where we are
> > dealing with one HDR and multiple SDR frames. Now, the HDR content
> > comes with its own metadata information, which needs to be passed to
> > the display using the AVI infoframes, for the correct HDR experience,
> > but we need to make others non-HDR frames also compatible to this brightness
> range of metadata, so we do tone mapping.
> 
> I've been thinking a bit about that, and currently my view is that this is not the best way
> of arranging things. MAX-CLL and MAX-FALL are specific to certain current source of
> HD imagery, and not all HDR sources will have those specific numbers, and video
> standards may change in the future (i.e. frame by frame meta-data). Exactly how they
> can be used is unclear - i.e. they are observations from a video stream, not parameters
> for a specific tone mapping. So punting them to the Compositor doesn't seem such a
> good approach.

I kindof agree on this point, that, the usage model is not very clear right now, and this stack and usage model will mature with time. But I would like to still keep this here, for following reasons:
 - If you closely see the HDMI AVI infoframes specified in CEA-861-G spec, the HDR metadata section is expecting MAX-CLL and MAX-FALL, this means many monitor might be using this already to provide you better viewing experience. So if a content can set these values, no harm in hand carrying that. 
- The tone mapping API implemented by libVA expects this data, as they are using it to weight the brightness values in frame. So the compositor will pass these values to libVA too.  
> 
> Instead what I'd suggest is providing some concrete tone mapping operators for HDR
> handling (at least for V1), with well defined behavior and parameters. The video
> playback application can do the conversion from source format specific observations
> like MAX-CLL and MAX-FALL into the specific compositor tone mapping parameters,
> or it has the option of determining those parameters some other way (user setting
> perhaps, or it's own analysis of the video stream), or it can even do the adaptation of
> the source to the display itself, since it has access to the display characteristics via the
> ICC profile.
> 
> I think it's quite possible to have the Compositor do sane HDR conversions with no
> other extra information than the HDR nominal diffuse white brightness for each HDR
> profile, and perhaps this could be a base Compositor tone mapping behavior, with
> other tone mapping algorithms (and their associated parameters) added as they are
> identified and defined or standardized.
> 
Yep, this is very close to what we are doing in our compositor code.
 
> >> If the idea is that somehow the compositor is to do dynamic HDR
> >> rendering, then I have my doubts about its suitability and
> >> practicality, unless you are prepared to spell out in detail the algorithm it should
> use for performing this.
> >>
> > The GL tone mapping algorithms will be opensource methods, and will be
> > available for the code review. HW tone mapping is using the libVA interface and
> media-driver handler.
> 
> Right, but there is a distinction between a particular implementation and a Compositor
> specification.
> 
> Can you write a specification of how the tone curves you are proposing to use work, in
> such a way that someone else can implement them, and that someone dealing with
> other HDR sources can use them ?
Sure, in fact, soon we will publish the code, and you can have a look and provide us feedback.

> (i.e. HDR photographs or synthetic imagery from games etc.)
> 
> > Right now we are not targeting this, but eventually we want to be
> > there. As we all know, it's a huge area to cover in an single attempt,
> > and gets very cumbersome. So the plan is to first implement a modular
> > limited capability HDR video playback, and  use that as a driving and
> > testing vehicle for color management, and then later enhance it with adding more
> and more modules.
> 
> Sure. I'm a little worried though, that video specific HDR considerations will end up
> getting set in stone, and hamper more general HDR compatibility.

I completely understand this concern, and it seems genuine too. But honestly, seeing the scope of complete work I feel like this is a Himalayan task, and modular approach is one practical way of achieving something gradually. With small and modular targets, we can slowly expand the stack, and we can try to make it as generic as it gets. I am pretty sure and hopeful that with inputs like we got here, and with appropriate code review, we should come  up with a promising software stack. 

- Shashank 
> 
> Cheers,
> 
> Graeme Gill.