[PATCH] color-management: Add HDR-metadata support

Tue Mar 19 06:16:17 UTC 2019

Hi Ankit,

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

> Hi Harish,
>
> Thanks for the correction and suggestions.
> Please find my responses inline:
>
> On 3/18/2019 9:40 PM, Harish Krupo wrote:
>> Hi Ankit,
>>
>> Nautiyal, Ankit K <ankit.k.nautiyal at intel.com> writes:
>>
>>> From: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>>
>>> This patch adds HDR-metadata support in the color-management-protocol.
>>> It enables a client to:
>>> - know if an output is HDR capable.
>>> - set HDR-metadata values, received from an HDR content, for a surface.
>>>
>>> The HDR-metadata values : MAX_CLL, MAX_FALL, MAX_LUMINANCE,
>>> MIN LUMINANCE can be sent from the client to compositor, which can
>>> inturn send these to kernel. Kernel can finally send these values in
>>
>> typo: in turn :)
>
> Thanks for catching this. :)
>
>>
>>> AVI-INFOFRAMES to the HDR display to provide an idea of the brightness
>>> of the content. The display can use this information to adapt itself
>>> for a better viewing experience.
>>
>> You can also add that this information could be used for tone mapping
>> when composing along with other buffers.
>>
>>>
>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>> ---
>>>  .../color-management-unstable-v1.xml               | 48 +++++++++++++++++++++-
>>>  1 file changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/unstable/color-management/color-management-unstable-v1.xml b/unstable/color-management/color-management-unstable-v1.xml
>>> index 7b4d08e..58a41a1 100644
>>> --- a/unstable/color-management/color-management-unstable-v1.xml
>>> +++ b/unstable/color-management/color-management-unstable-v1.xml
>>> @@ -119,7 +119,15 @@
>>>        <arg name="id" type="new_id" interface="zwp_color_space_v1"/>
>>>      </request>
>>>
>>> -    <!-- TODO: HDR capabilities event -->
>>> +    <event name="hdr_capability">
>>> +      <description summary="HDR Capability">
>>> +	This event is sent after creating a zwp_color_management_output
>>> +	(see zwp_color_manager.get_color_management_output) and tells about the
>>> +	HDR capability of an output. A value of '1' indicates that the output
>>> +	is HDR capable, and '0' indicates a non-HDR output.
>>
>> Can the client span across outputs? If so what will the client receive?
>> 1 or 0?
>
> The interface zwp_color_management_output is associated with a single output. If
> a client spans across multiple outputs, it will have to get two different
> zwp_color_management for each of the outputs.
> Each might provide a separate value, (as in case of one HDR and one non HDR
> display), client can still set the hdr-metadata for the surface. It depends on
> the compositor as to what it wants to do with the given values.
>

Yeah. My bad.
A different question this time :)
The event "whether an output supports HDR or not" seems to be lacking.
What can the client do with this information? Maybe document the example
behaviour?
Typically the client would need the HDR display's metadata if it were to
do something meaningful like apply tone mapping or such. But this might
be complicated when the client spans multiple outputs.

>>
>>> +      </description>
>>> +      <arg name="hdr_support" type="uint"/>
>>> +    </event>
>>>
>>>      <request name="destroy" type="destructor">
>>>        <description summary="destroy the color management output">
>>> @@ -171,7 +179,43 @@
>>>        <arg name="render_intent" type="uint" enum="render_intent"/>
>>>      </request>
>>>
>>> -    <!-- TODO: HDR metadata request -->
>>> +    <request name="set_hdr_metadata">
>>> +      <description summary="set the hdr metadata for the surface">
>>> +	Set the HDR-metadata for the underlying surface. The HDR-metadata is
>>> +	double buffered, and will be applied at the time wl_surface.commit of
>>> +	the corresponding wl_surface is called.
>>> +	The HDR-metadata constitutes of MAX-CLL, MAX-FALL, Max Luminance and
>>> +	Min Luminance as defined by SMPTE ST.2086. The clients get these values
>>> +	for an HDR video via ffmpeg 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. All except for Minimum Luminace, can be represented by integer,
>>> +	as they take values of the order of hundreds of nits.
>>> +	For Minimum Luminance, fixed type is used so that it can take smaller
>>> +	decimal values, which can be converted to and from double.
>>> +
>>> +	For setting color-primaries as part of HDR-metadata, the client should
>>> +	manufacture the icc profile and then use the zwp_color_manager interface
>>> +	to get the color-space, (see zwp_color_manager.create_color_space).
>>> +	The request set_color_space can then be used to set the color-space.
>>> +
>>> +	A client may request for setting the HDR-metadata for a surface, even if
>>> +	there is no HDR-capable output, compositor should handle such cases.
>>> +
>>
>> "Should" sounds a bit assertive. Maybe drop the sentence?
>>
>
> I am fine with that, provided, its implicitly understood that client is free to
> request for setting the HDR-metadata, in spite of being displayed on non-HDR
> display and no error events would be sent for such a case.
>

The client is indeed free to set the HDR metadata for a surface. The
compositor can choose to act on it or ignore it. That is why I felt that
the statement "compositor should handle such cases" was a bit assertive.

>>> +	The HDR-metadata should be initialized with '-1' for a surface,
>>> +	signifying that the values are invalid. So, If a client does not set
>>> +	hdr-metadata values for a surface, i.e. does not request
>>> +	set_hdr_metadata default value of '-1' will be there, telling the
>>> +	compositor that there is no HDR-metadata for that surface.
>>> +      </description>
>>> +      <arg name="max_cll" type="uint"/>
>>> +      <arg name="max_fall" type="uint"/>
>>> +      <arg name="max_luminance" type="uint"/>
>>
>> max_cll and max_fall are uints but max_luminance is of fixed type.
>> Also, what happens when the clients wants to reset the hdr metadata?
>> Would it set -1 to all the values to notify of this change?
>> I think all of them should be of fixed types to be able to send -1.
>>
> Good point! We can have all these as fixed type.
> But having all arguments as fixed will add an additional over-head of converting
> to and from one type to another, even for max_cll and max_fall values.
> Also, it opens a window for client to send other invalid values for max_cll and
> max_fall.
> An alternative can be an additional request 'reset_hdr_metadata'.
> explicitly asking for resetting the values, without giving any values.
>

That would work too. Another approach would be to do something similar
to this: [1].
Create a new object for HDR, add your hdr_metadata request there and add
a destroy request so that the client can notify that the surface doesn't
have HDR data anymore.

Thank you
Regards
Harish Krupo

[1] https://patchwork.freedesktop.org/patch/291644/?series=57852&rev=2