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

Tue Mar 19 05:52:50 UTC 2019

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.

>
>> +      </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 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.

Regards,
Ankit

>> +      <arg name="min_luminance" type="fixed"/>
>> +    </request>
>>
>>      <event name="preferred_color_space">
>>        <description summary="preferred color space">
>
> Thank you
> Regards
> Harish Krupo
>