[PATCH] unstable: add HDR Mastering Meta-data Protocol
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Fri Mar 1 09:10:16 UTC 2019
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 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.
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 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.
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20190301/61c8fe09/attachment-0001.html>
More information about the wayland-devel
mailing list