[PATCH] unstable: add HDR Mastering Meta-data Protocol
Pekka Paalanen
ppaalanen at gmail.com
Thu Feb 28 14:21:09 UTC 2019
On Wed, 27 Feb 2019 10:27:16 +0530
"Nautiyal, Ankit K" <ankit.k.nautiyal at intel.com> wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal at 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 at intel.com>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at 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 at intel.com>
> +Harish Krupo <harish.krupo.kps at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190228/51dae490/attachment-0001.sig>
More information about the wayland-devel
mailing list