Re: [PATCH v2 5/6] [media] vivid: Add support for HSV formats

[Hans Verkuil]

On 07/15/2016 06:13 PM, Ricardo Ribalda Delgado wrote:
> This patch adds support for V4L2_PIX_FMT_HSV24 and V4L2_PIX_FMT_HSV32.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> ---
>  drivers/media/common/v4l2-tpg/v4l2-tpg-core.c   | 94 +++++++++++++++++++++++--
>  drivers/media/platform/vivid/vivid-vid-common.c | 14 ++++
>  include/media/v4l2-tpg.h                        |  1 +
>  3 files changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> index acf0e6854832..85b9c1925dd9 100644
> --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c
> @@ -318,6 +318,10 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
>  		tpg->hmask[0] = ~1;
>  		tpg->color_representation = TGP_COLOR_REPRESENTATION_YUV;
>  		break;
> +	case V4L2_PIX_FMT_HSV24:
> +	case V4L2_PIX_FMT_HSV32:
> +		tpg->color_representation = TGP_COLOR_REPRESENTATION_HSV;
> +		break;
>  	default:
>  		return false;
>  	}
> @@ -351,6 +355,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
>  		break;
>  	case V4L2_PIX_FMT_RGB24:
>  	case V4L2_PIX_FMT_BGR24:
> +	case V4L2_PIX_FMT_HSV24:
>  		tpg->twopixelsize[0] = 2 * 3;
>  		break;
>  	case V4L2_PIX_FMT_BGR666:
> @@ -361,6 +366,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
>  	case V4L2_PIX_FMT_ARGB32:
>  	case V4L2_PIX_FMT_ABGR32:
>  	case V4L2_PIX_FMT_YUV32:
> +	case V4L2_PIX_FMT_HSV32:
>  		tpg->twopixelsize[0] = 2 * 4;
>  		break;
>  	case V4L2_PIX_FMT_NV12:
> @@ -408,6 +414,7 @@ bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc)
>  		tpg->twopixelsize[1] = 4;
>  		break;
>  	}
> +
>  	return true;
>  }
>  EXPORT_SYMBOL_GPL(tpg_s_fourcc);
> @@ -490,6 +497,64 @@ static inline int linear_to_rec709(int v)
>  	return tpg_linear_to_rec709[v];
>  }
>  
> +static void color_to_hsv(struct tpg_data *tpg, int r, int g, int b,
> +			   int *h, int *s, int *v)
> +{
> +	int max_rgb, min_rgb, diff_rgb;
> +	int aux;
> +	int third;
> +
> +	r >>= 4;
> +	g >>= 4;
> +	b >>= 4;
> +
> +	/*V*/

Please add a space after /* and before */.
I also think it is better to write Value, Saturation, Hue instead of
V, S, H.

> +	max_rgb = max3(r, g, b);
> +	*v = max_rgb;
> +	if (!max_rgb) {
> +		*h = 0;
> +		*s = 0;
> +		return;
> +	}
> +
> +	/*S*/
> +	min_rgb = min3(r, g, b);
> +	diff_rgb = max_rgb - min_rgb;
> +	aux = 255 * diff_rgb;
> +	aux += max_rgb / 2;
> +	aux /= max_rgb;
> +	*s = aux;
> +	if (!aux) {
> +		*h = 0;
> +		return;
> +	}
> +
> +	/*H*/
> +	if (max_rgb == r) {
> +		aux =  g - b;
> +		third = 0;
> +	} else if (max_rgb == g) {
> +		aux =  b - r;
> +		third = 60;
> +	} else {
> +		aux =  r - g;
> +		third = 120;
> +	}
> +
> +	aux *= 30;
> +	aux += diff_rgb / 2;
> +	aux /= diff_rgb;
> +	aux += third;
> +
> +	/*Clamp H*/
> +	if (aux < 0)
> +		aux += 180;
> +	else if (aux > 180)
> +		aux -= 180;
> +	*h = aux;
> +
> +}
> +
>  static void rgb2ycbcr(const int m[3][3], int r, int g, int b,
>  			int y_offset, int *y, int *cb, int *cr)
>  {
> @@ -829,7 +894,19 @@ static void precalculate_color(struct tpg_data *tpg, int k)
>  		ycbcr_to_color(tpg, y, cb, cr, &r, &g, &b);
>  	}
>  
> -	if (tpg->color_representation == TGP_COLOR_REPRESENTATION_YUV) {
> +	switch (tpg->color_representation) {
> +	case TGP_COLOR_REPRESENTATION_HSV:
> +	{
> +		int h, s, v;
> +
> +		color_to_hsv(tpg, r, g, b, &h, &s, &v);
> +		tpg->colors[k][0] = h;
> +		tpg->colors[k][1] = s;
> +		tpg->colors[k][2] = v;

Would quantization (limited/full range) be relevant here? I don't know if limited
range would make sense (or what those limits would be).

Regards,

	Hans