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

Tue Mar 19 14:10:45 UTC 2019

Pekka Paalanen <ppaalanen at gmail.com> writes:

> On Tue, 19 Mar 2019 11:46:17 +0530
> Harish Krupo <harish.krupo.kps at intel.com> wrote:
>
>> 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.
>
> Hi,
>
> this should include the monitor's HDR metadata indeed. Otherwise the
> client cannot do the tone mapping specific to the output which would
> allow direct scanout of the client buffer in the best case.
>
> You could have two events: "sdr" without any metadata, and "hdr" with
> arguments. Clients should be prepared for the data to change on the
> fly, if the compositor e.g. decides to force SDR mode after being in
> HDR mode on the output. It's the same thing as what the
> color_space_changed event signals, except color space is a complex
> object rather than just some numbers.
>

Hi Pekka,

One more thing: HDR is primarlity characterized by its encoding curve (PQ or
HLG). The luminance values are mostly used to tone mapping. This means
it is possible to have HDR content without all the below values defined.
(We have seen some of the HDR encoded content without all the luminance
values defined.)
AFAIK the ICC profile defines the transfer curves as luts (please
correct me if I am wrong). While sending the HDR metadata information to
the kernel we need to set the eotf type bit. I am not sure how we can
get this transfer curve info from the lut. FFMPEG provides a way to
extract this, here: [1].
So I think we need to add another field in this protocol which sets the
transfer curves for known eotfs.

Thank you
Regards
Harish Krupo

[1] https://www.ffmpeg.org/doxygen/4.1/pixfmt_8h.html#ad4791ea14975f098b649db7fcd731ce6

>> >>  
>> >>> +      </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.
>
> No, the compositor cannot choose to ignore it.
>
> The compositor must always honour the client provided image metadata,
> if the compositor exposes this interface. It is not a hint.
>
> I agree that it must not be an error to set up HDR metadata without HDR
> outputs. That will just lead to the compositor converting to SDR the
> best it can.
>
>> >>> +	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.
>
> Rather than talking about -1, how about rewording that to SDR? It
> doesn't matter how a compositor stores the state.
>
>> >>> +      </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.
>
> Is there a use case for removing HDR metadata completely independently
> to a color profile? I would not expect so, so you can just rely on the
> zwp_color_management_surface_v1 destructor to remove the HDR metadata
> as well.
>
> But if that does seem useful, add another request to reset HDR metadata
> to SDR. That is much more clear than dedicating some illegal values to
> "turn it off".
>

> I see no issue in using wl_fixed all the way. The overhead of data type
> conversion is insignificant, I challenge you to make it show up in any
> performance profile that uses libwayland messaging, even for completely
> artificial test cases. You need to check for invalid values with uint
> too, I would not expect zero to be a valid value for all, or min value
> being greater than max value.
>
> Don't forget error codes for invalid values.
>
>
> Thanks,
> pq
>
>> [1] https://patchwork.freedesktop.org/patch/291644/?series=57852&rev=2