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

Tue Mar 19 11:54:56 UTC 2019

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.

> >>  
> >>> +      </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
-------------- 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/20190319/84c4198f/attachment-0001.sig>