<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Pekka, Thanks for the comments and suggestions. Please my responses inline: <div class="moz-cite-prefix">On 2/28/2019 7:51 PM, Pekka Paalanen wrote: </div> <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap="">On Wed, 27 Feb 2019 10:27:16 +0530 "Nautiyal, Ankit K" <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">From: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> 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] <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/patch/134570/">https://patchwork.freedesktop.org/patch/134570/</a> [2] <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/patch/286062/">https://patchwork.freedesktop.org/patch/286062/</a> Co-authored-by: Harish Krupo <a class="moz-txt-link-rfc2396E" href="mailto:harish.krupo.kps@intel.com"><harish.krupo.kps@intel.com></a> Signed-off-by: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> </pre> </blockquote> <pre wrap="">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. </pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <blockquote type="cite"> <pre wrap="">--- 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 <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> +Harish Krupo <a class="moz-txt-link-rfc2396E" href="mailto:harish.krupo.kps@intel.com"><harish.krupo.kps@intel.com></a> 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 </pre> </blockquote> <pre wrap="">Could it be named hdr-mastering rather than hdr-mastering-metadata? Shorter C function names wouldn't hurt.</pre> </blockquote> Yes I guess, thanks for the suggestion, or perhaps "hdr-metadata" ? <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">@@ -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"> </pre> </blockquote> <pre wrap="">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.</pre> </blockquote> Point taken. Will make the commit concise and elaborate more in protocol documentation. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ 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. </pre> </blockquote> <pre wrap="">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?</pre> </blockquote> 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? <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> 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.</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap="">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.</pre> </blockquote> As discussed above, seems best to have a separate protocol for these parameters. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> 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?</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> 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.</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> 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?</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ </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. </pre> </blockquote> <pre wrap="">What side-effects does this have?</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ </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. </pre> </blockquote> <pre wrap="">What happens if the wl_surface is destroyed first? And the client issues requests through this interface?</pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ </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. </pre> </blockquote> <pre wrap="">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. </pre> </blockquote> 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. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <blockquote type="cite"> <pre wrap="">+ </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"/> </pre> </blockquote> <pre wrap="">What are the units for all these values? Do integers give enough precision and range?</pre> </blockquote> The values are in nits, AFAIK the values are of the order of hundreds and thousands(for max values) So perhaps uint is sufficient. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap="">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?</pre> </blockquote> Not sure about this, perhaps more parameters get added. <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> OTOH, keeping all parameters in a single request means that there is no error case of not defining all the parameters.</pre> </blockquote> <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ </request> + + <request name="destroy" type="destructor"> + <description summary="destroy the hdr mastering surface object"> + Destroy the hdr_surface. </pre> </blockquote> <pre wrap="">What effects does this have to the wl_surface?</pre> </blockquote> 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 <blockquote cite="mid:20190228162109.1e0f5092@eldfell.localdomain" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ </description> + </request> + </interface> +</protocol> </pre> </blockquote> <pre wrap=""> Thanks, pq </pre> </blockquote> </body> </html>