Hi Hans, On 3/16/21 2:16 PM, Hans Verkuil wrote: > On 09/02/2021 17:24, Stanimir Varbanov wrote: >> Introduce Content light level and Mastering display colour >> volume Colorimetry compound controls with relevant payload >> structures and validation. >> >> Signed-off-by: Stanimir Varbanov <[email protected]> >> --- >> drivers/media/v4l2-core/v4l2-ctrls.c | 67 ++++++++++++++++++++++++++++ >> include/media/v4l2-ctrls.h | 4 ++ >> include/uapi/linux/v4l2-controls.h | 31 +++++++++++++ >> include/uapi/linux/videodev2.h | 3 ++ >> 4 files changed, 105 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c >> b/drivers/media/v4l2-core/v4l2-ctrls.c >> index 335cf354f51b..8bd3cf0e1e4f 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >> @@ -1205,6 +1205,8 @@ const char *v4l2_ctrl_get_name(u32 id) >> /* Colorimetry controls */ >> /* Keep the order of the 'case's the same as in v4l2-controls.h! */ >> case V4L2_CID_COLORIMETRY_CLASS: return "Colorimetry Controls"; >> + case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO: return "HDR10 >> Content Light Info"; >> + case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: return "HDR10 >> Mastering Display"; >> default: >> return NULL; >> } >> @@ -1491,6 +1493,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum >> v4l2_ctrl_type *type, >> *type = V4L2_CTRL_TYPE_AREA; >> *flags |= V4L2_CTRL_FLAG_READ_ONLY; >> break; >> + case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO: >> + *type = V4L2_CTRL_TYPE_HDR10_CLL_INFO; >> + break; >> + case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY: >> + *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY; >> + break; >> default: >> *type = V4L2_CTRL_TYPE_INTEGER; >> break; >> @@ -1786,6 +1794,12 @@ static void std_log(const struct v4l2_ctrl *ctrl) >> case V4L2_CTRL_TYPE_FWHT_PARAMS: >> pr_cont("FWHT_PARAMS"); >> break; >> + case V4L2_CTRL_TYPE_HDR10_CLL_INFO: >> + pr_cont("HDR10_CLL_INFO"); >> + break; >> + case V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY: >> + pr_cont("HDR10_MASTERING_DISPLAY"); >> + break; >> default: >> pr_cont("unknown type %d", ctrl->type); >> break; >> @@ -1838,6 +1852,7 @@ static int std_validate_compound(const struct >> v4l2_ctrl *ctrl, u32 idx, >> struct v4l2_ctrl_hevc_sps *p_hevc_sps; >> struct v4l2_ctrl_hevc_pps *p_hevc_pps; >> struct v4l2_ctrl_hevc_slice_params *p_hevc_slice_params; >> + struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering; >> struct v4l2_area *area; >> void *p = ptr.p + idx * ctrl->elem_size; >> unsigned int i; >> @@ -2133,6 +2148,52 @@ static int std_validate_compound(const struct >> v4l2_ctrl *ctrl, u32 idx, >> zero_padding(*p_hevc_slice_params); >> break; >> >> + case V4L2_CTRL_TYPE_HDR10_CLL_INFO: >> + break; >> + >> + case V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY: >> + p_hdr10_mastering = p; >> + >> + for (i = 0; i < 3; ++i) { >> + if (p_hdr10_mastering->display_primaries_x[i] < >> + V4L2_HDR10_MASTERING_PRIMARIES_X_LOW || >> + p_hdr10_mastering->display_primaries_x[i] > >> + V4L2_HDR10_MASTERING_PRIMARIES_X_HIGH || >> + p_hdr10_mastering->display_primaries_y[i] < >> + V4L2_HDR10_MASTERING_PRIMARIES_Y_LOW || >> + p_hdr10_mastering->display_primaries_y[i] > >> + V4L2_HDR10_MASTERING_PRIMARIES_Y_HIGH) >> + return -EINVAL; >> + } >> + >> + if (p_hdr10_mastering->white_point_x < >> + V4L2_HDR10_MASTERING_WHITE_POINT_X_LOW || >> + p_hdr10_mastering->white_point_x > >> + V4L2_HDR10_MASTERING_WHITE_POINT_X_HIGH || >> + p_hdr10_mastering->white_point_y < >> + V4L2_HDR10_MASTERING_WHITE_POINT_Y_LOW || >> + p_hdr10_mastering->white_point_y > >> + V4L2_HDR10_MASTERING_WHITE_POINT_Y_HIGH) >> + return -EINVAL; >> + >> + if (p_hdr10_mastering->max_display_mastering_luminance < >> + V4L2_HDR10_MASTERING_MAX_LUMA_LOW || >> + p_hdr10_mastering->max_display_mastering_luminance > >> + V4L2_HDR10_MASTERING_MAX_LUMA_HIGH || >> + p_hdr10_mastering->min_display_mastering_luminance < >> + V4L2_HDR10_MASTERING_MIN_LUMA_LOW || >> + p_hdr10_mastering->min_display_mastering_luminance > >> + V4L2_HDR10_MASTERING_MIN_LUMA_HIGH) >> + return -EINVAL; >> + >> + if (p_hdr10_mastering->max_display_mastering_luminance == >> + V4L2_HDR10_MASTERING_MAX_LUMA_LOW && >> + p_hdr10_mastering->min_display_mastering_luminance == >> + V4L2_HDR10_MASTERING_MIN_LUMA_HIGH) > > I had to think about this one :-) > > Isn't it clearer to write: > > if (p_hdr10_mastering->min_display_mastering_luminance >= > p_hdr10_mastering->max_display_mastering_luminance) > > (even though it can't be >, but >= is probably more robust and future proof) > > And is it indeed invalid if both are the same?
This what the ITU-T Rec. H.265 spec says: "When max_display_mastering_luminance is equal to 50 000, min_display_mastering_luminance shall not be equal to 50 000." -- regards, Stan
