Re: [PATCH v3 1/4] [media] exynos-gsc: Use 576p instead 720p as a threshold for colorspaces

[Hans Verkuil]

On 02/10/2017 05:37 PM, Thibault Saunier wrote:
> On 02/10/2017 12:07 PM, Hans Verkuil wrote:
>> On 02/10/2017 03:10 PM, Thibault Saunier wrote:
>>> From: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>>>
>>> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
>>> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers
>>> don't agree on the display resolution that should be used as a threshold.
>>>
>>> Some drivers set V4L2_COLORSPACE_REC709 for 720p and higher while others
>>> set V4L2_COLORSPACE_REC709 for anything higher than 576p. Newers drivers
>>> use the latter and that also matches what user-space multimedia programs
>>> do (i.e: GStreamer), so change the driver logic to be aligned with this.
>>>
>>> Also, check for the resolution in G_FMT instead unconditionally setting
>>> the V4L2_COLORSPACE_REC709 colorspace.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
>>> Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
>>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@xxxxxxxxxxxxxxx>
>>> ---
>>>
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx>'
>>>
>>> Changes in v2: None
>>>
>>>   drivers/media/platform/exynos-gsc/gsc-core.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>>> index 59a634201830..db7d9883861b 100644
>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>>> @@ -472,7 +472,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>>   
>>>   	pix_mp->num_planes = fmt->num_planes;
>>>   
>>> -	if (pix_mp->width >= 1280) /* HD */
>>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>>>   		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>>>   	else /* SD */
>>>   		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> @@ -519,9 +519,13 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>>   	pix_mp->height		= frame->f_height;
>>>   	pix_mp->field		= V4L2_FIELD_NONE;
>>>   	pix_mp->pixelformat	= frame->fmt->pixelformat;
>>> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>>>   	pix_mp->num_planes	= frame->fmt->num_planes;
>>>   
>>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>>> +		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>>> +	else /* SD */
>>> +		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> +
>>>   	for (i = 0; i < pix_mp->num_planes; ++i) {
>>>   		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
>>>   			frame->fmt->depth[i]) / 8;
>>>
>> This is a mem2mem device, right? In the case of mem2mem devices the driver should never
>> set the colorspace, instead it just copies it from what the application provides (the
>> video output side) to the capture side.
>>
>> After all, you are just scaling here so the input and output colorspaces are
>> exactly the same, and the scaler doesn't care what the colorspace is.
> 
> This device also does color conversion so I think it matters here, am I 
> misunderstanding something?

It almost certainly only does color encoding conversion (e.g. from RGB to YCbCr or
vice versa, and from limited to full range or vice versa).

That only affects the ycbcr_enc and quantization fields (although these probably
will just remain 0). The colorspace and xfer_func values remain constant since
the HW is unlikely to support full colorspace conversions.

See https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html for more
information on what colorspaces really are.

> Also, is that comment only for the try_fmt part as it looks like in 
> g_fmt the driver should fill up the structure
> with its values?

Yes, same for try_fmt.

Regards,

	Hans