| Message ID | 20200325173456.21455-1-slongerbeam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: utils: Default colorspace to SRGB | expand |
Hi Steve, On 25/03/2020 18:34, Steve Longerbeam wrote: > The function init_mbus_colorimetry() is used to initialize the imx > subdevice mbus colorimetry to some sane defaults when the subdevice is > registered. Currently it guesses at a colorspace based on the passed > mbus pixel format. If the format is RGB, it chooses colorspace > V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses > V4L2_COLORSPACE_SMPTE170M. > > While that might be a good guess, it's not necessarily true that a RGB > pixel format encoding uses a SRGB colorspace, or that a YUV encoding > uses a SMPTE170M colorspace. Instead of making this dubious guess, > just default the colorspace to SRGB. > > Reported-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > drivers/staging/media/imx/imx-media-utils.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c > index fae981698c49..8344675bfd05 100644 > --- a/drivers/staging/media/imx/imx-media-utils.c > +++ b/drivers/staging/media/imx/imx-media-utils.c > @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = { > static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus, > const struct imx_media_pixfmt *fmt) > { > - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ? > - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M; > + mbus->colorspace = V4L2_COLORSPACE_SRGB; > mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace); > mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace); > mbus->quantization = The quantization is probably wrong as well since it checks fmt->cs. The first argument should just be 'true'. > In any case, this patch no longer applies after the imx utils patch series. Regards, Hans
Hi Hans, On 4/16/20 3:52 AM, Hans Verkuil wrote: > Hi Steve, > > On 25/03/2020 18:34, Steve Longerbeam wrote: >> The function init_mbus_colorimetry() is used to initialize the imx >> subdevice mbus colorimetry to some sane defaults when the subdevice is >> registered. Currently it guesses at a colorspace based on the passed >> mbus pixel format. If the format is RGB, it chooses colorspace >> V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses >> V4L2_COLORSPACE_SMPTE170M. >> >> While that might be a good guess, it's not necessarily true that a RGB >> pixel format encoding uses a SRGB colorspace, or that a YUV encoding >> uses a SMPTE170M colorspace. Instead of making this dubious guess, >> just default the colorspace to SRGB. >> >> Reported-by: Hans Verkuil <hverkuil@xs4all.nl> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> drivers/staging/media/imx/imx-media-utils.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c >> index fae981698c49..8344675bfd05 100644 >> --- a/drivers/staging/media/imx/imx-media-utils.c >> +++ b/drivers/staging/media/imx/imx-media-utils.c >> @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = { >> static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus, >> const struct imx_media_pixfmt *fmt) >> { >> - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ? >> - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M; >> + mbus->colorspace = V4L2_COLORSPACE_SRGB; >> mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace); >> mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace); >> mbus->quantization = > The quantization is probably wrong as well since it checks fmt->cs. > The first argument should just be 'true'. I don't think so. The default quantization depends in part on whether the pixel format is RGB or YUV, which is the reason for passing boolean (fmt->cs == IPUV3_COLORSPACE_RGB) as the first argument. I realize "fmt->cs" is a misnomer, but it is borrowing from earlier misnomers, i.e. the 'enum ipu_color_space' enumerations. Fixing those names would touch lots of imx driver code. > > In any case, this patch no longer applies after the imx utils patch series. Ok, I'll re-submit after the utils patch series is merged. Steve
On 16/04/2020 19:10, Steve Longerbeam wrote: > Hi Hans, > > On 4/16/20 3:52 AM, Hans Verkuil wrote: >> Hi Steve, >> >> On 25/03/2020 18:34, Steve Longerbeam wrote: >>> The function init_mbus_colorimetry() is used to initialize the imx >>> subdevice mbus colorimetry to some sane defaults when the subdevice is >>> registered. Currently it guesses at a colorspace based on the passed >>> mbus pixel format. If the format is RGB, it chooses colorspace >>> V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses >>> V4L2_COLORSPACE_SMPTE170M. >>> >>> While that might be a good guess, it's not necessarily true that a RGB >>> pixel format encoding uses a SRGB colorspace, or that a YUV encoding >>> uses a SMPTE170M colorspace. Instead of making this dubious guess, >>> just default the colorspace to SRGB. >>> >>> Reported-by: Hans Verkuil <hverkuil@xs4all.nl> >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >>> --- >>> drivers/staging/media/imx/imx-media-utils.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c >>> index fae981698c49..8344675bfd05 100644 >>> --- a/drivers/staging/media/imx/imx-media-utils.c >>> +++ b/drivers/staging/media/imx/imx-media-utils.c >>> @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = { >>> static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus, >>> const struct imx_media_pixfmt *fmt) >>> { >>> - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ? >>> - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M; >>> + mbus->colorspace = V4L2_COLORSPACE_SRGB; >>> mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace); >>> mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace); >>> mbus->quantization = >> The quantization is probably wrong as well since it checks fmt->cs. >> The first argument should just be 'true'. > > I don't think so. The default quantization depends in part on whether > the pixel format is RGB or YUV, which is the reason for passing boolean > (fmt->cs == IPUV3_COLORSPACE_RGB) as the first argument. Ah yes, that's true. Sorry for the noise. Regards, Hans > > I realize "fmt->cs" is a misnomer, but it is borrowing from earlier > misnomers, i.e. the 'enum ipu_color_space' enumerations. Fixing those > names would touch lots of imx driver code. > > >> >> In any case, this patch no longer applies after the imx utils patch series. > > Ok, I'll re-submit after the utils patch series is merged. > > Steve >
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c index fae981698c49..8344675bfd05 100644 --- a/drivers/staging/media/imx/imx-media-utils.c +++ b/drivers/staging/media/imx/imx-media-utils.c @@ -236,8 +236,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = { static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus, const struct imx_media_pixfmt *fmt) { - mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ? - V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M; + mbus->colorspace = V4L2_COLORSPACE_SRGB; mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace); mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace); mbus->quantization =
The function init_mbus_colorimetry() is used to initialize the imx subdevice mbus colorimetry to some sane defaults when the subdevice is registered. Currently it guesses at a colorspace based on the passed mbus pixel format. If the format is RGB, it chooses colorspace V4L2_COLORSPACE_SRGB, and if the format is YUV, it chooses V4L2_COLORSPACE_SMPTE170M. While that might be a good guess, it's not necessarily true that a RGB pixel format encoding uses a SRGB colorspace, or that a YUV encoding uses a SMPTE170M colorspace. Instead of making this dubious guess, just default the colorspace to SRGB. Reported-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- drivers/staging/media/imx/imx-media-utils.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)