Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585673 > unrolled thread

[PATCH v5 0/3] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 0/3] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers Thibault Saunier <thibault.saunier@osg.samsung.com> - 2017-02-21 20:30 +0100
    [PATCH v5 2/3] [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT if DEFAULT provided Thibault Saunier <thibault.saunier@osg.samsung.com> - 2017-02-21 20:30 +0100
      Re: [PATCH v5 2/3] [media] s5p-mfc: Set colorspace in  VIDIO_{G,TRY}_FMT if DEFAULT provided Andrzej Hajda <a.hajda@samsung.com> - 2017-02-23 15:00 +0100

#1585673 — [PATCH v5 0/3] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

Hello,

This patchset fixes a few issues on the colorspace logic for the exynos-gsc
and s5p-mfc drivers.

We now handle the colorspace in those drivers, and make sure to respect user setting if
possible.

We also now set the 'v4l2_pix_format:field' if userspace passed ANY, avoiding GStreamer
spamming error at us about the driver not following the standard.

This is the fifth version of the patch serie.

Best regards,

Thibault Saunier

Changes in v5:
- Squash commit to always use output colorspace on the capture side
  inside this one
- Fix typo in commit message
- Just adapt the field and never error out.

Changes in v4:
- Reword commit message to better back our assumptions on specifications
- Set the colorspace only if the user passed V4L2_COLORSPACE_DEFAULT, in
  all other cases just use what userspace provided.

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@samsung.com>'
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

Thibault Saunier (3):
  [media] exynos-gsc: Use user configured colorspace if provided
  [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT if DEFAULT
    provided
  [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in
    try_fmt

 drivers/media/platform/exynos-gsc/gsc-core.c | 20 +++++++++++++++-----
 drivers/media/platform/exynos-gsc/gsc-core.h |  1 +
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 17 +++++++++++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

-- 
2.11.1

[toc] | [next] | [standalone]

#1585675 — [PATCH v5 2/3] [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT if DEFAULT provided

The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV but the driver
didn't set the colorimetry when userspace provided
V4L2_COLORSPACE_DEFAULT.

Use 576p display resolution as a threshold to set this as suggested by
EIA CEA 861B.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v5: None
Changes in v4:
- Set the colorspace only if the user passed V4L2_COLORSPACE_DEFAULT, in
  all other cases just use what userspace provided.

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in

Changes in v2: None

 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 367ef8e8dbf0..0976c3e0a5ce 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -354,6 +354,11 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		pix_mp->plane_fmt[0].sizeimage = ctx->luma_size;
 		pix_mp->plane_fmt[1].bytesperline = ctx->buf_width;
 		pix_mp->plane_fmt[1].sizeimage = ctx->chroma_size;
+
+		if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
+			pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+		else /* SD */
+			pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		/* This is run on OUTPUT
 		   The buffer contains compressed image
@@ -378,6 +383,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 {
 	struct s5p_mfc_dev *dev = video_drvdata(file);
+	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct s5p_mfc_fmt *fmt;
 
 	mfc_debug(2, "Type is %d\n", f->type);
@@ -405,6 +411,14 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			mfc_err("Unsupported format by this MFC version.\n");
 			return -EINVAL;
 		}
+
+		if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) {
+			if (pix_mp->width > 720 &&
+					pix_mp->height > 576) /* HD */
+				pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+			else /* SD */
+				pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+		}
 	}
 
 	return 0;
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1586902 — Re: [PATCH v5 2/3] [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT if DEFAULT provided

On 21.02.2017 20:20, Thibault Saunier wrote:
> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV but the driver
> didn't set the colorimetry when userspace provided
> V4L2_COLORSPACE_DEFAULT.
>
> Use 576p display resolution as a threshold to set this as suggested by
> EIA CEA 861B.
>
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>
> ---
>
> Changes in v5: None
> Changes in v4:
> - Set the colorspace only if the user passed V4L2_COLORSPACE_DEFAULT, in
>   all other cases just use what userspace provided.
>
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> - Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in
>
> Changes in v2: None
>
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..0976c3e0a5ce 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -354,6 +354,11 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		pix_mp->plane_fmt[0].sizeimage = ctx->luma_size;
>  		pix_mp->plane_fmt[1].bytesperline = ctx->buf_width;
>  		pix_mp->plane_fmt[1].sizeimage = ctx->chroma_size;
> +
> +		if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
> +			pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> +		else /* SD */
> +			pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;

Again, it seems much more complicated for decoder, I suppose colorspace
field should be filled according to decoded information from the header
of byte stream. It looks like MFC does not parse  stream header for such
info. So it should be done either by the driver, either by userspace, if
userspace is able to get such info. For example in H.264 this info is
encoded in VUI header [1], I do not know about other codecs. I guess the
best solution for now is to not touch this field unless you can retrieve
this info from MFC.

[1]:
https://github.com/copiousfreetime/mp4parser/blob/master/isoparser/src/main/java/com/googlecode/mp4parser/h264/model/SeqParameterSet.java#L227

Regards
Andrzej

>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* This is run on OUTPUT
>  		   The buffer contains compressed image
> @@ -378,6 +383,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  {
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>  	struct s5p_mfc_fmt *fmt;
>  
>  	mfc_debug(2, "Type is %d\n", f->type);
> @@ -405,6 +411,14 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  			mfc_err("Unsupported format by this MFC version.\n");
>  			return -EINVAL;
>  		}
> +
> +		if (pix_mp->colorspace == V4L2_COLORSPACE_DEFAULT) {
> +			if (pix_mp->width > 720 &&
> +					pix_mp->height > 576) /* HD */
> +				pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> +			else /* SD */
> +				pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +		}
>  	}
>  
>  	return 0;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web